[API-NEXT,PATCHv2] linux-generic: _fdserver: allocating data table dynamicaly

Message ID 1473770446-54915-1-git-send-email-christophe.milard@linaro.org
State New
Headers show

Commit Message

Christophe Milard Sept. 13, 2016, 12:40 p.m.
The table containing the saved file-descriptors<->{context, key} couples is
now dynamically malloc'd in the fd server process, hence avoiding
the memory waste which happened in other process when the table was
staticaly reserved in all processes.

Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

---
 platform/linux-generic/_fdserver.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.7.4

Comments

Bill Fischofer Sept. 13, 2016, 12:02 p.m. | #1
This looks good now. Only question is whether you need to worry about
cache-aligning the table, but that's a tuning option that can be considered
later if needed.

On Tue, Sep 13, 2016 at 7:40 AM, Christophe Milard <
christophe.milard@linaro.org> wrote:

> The table containing the saved file-descriptors<->{context, key} couples is

> now dynamically malloc'd in the fd server process, hence avoiding

> the memory waste which happened in other process when the table was

> staticaly reserved in all processes.

>

> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

>


Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>



> ---

>  platform/linux-generic/_fdserver.c | 14 +++++++++++++-

>  1 file changed, 13 insertions(+), 1 deletion(-)

>

> diff --git a/platform/linux-generic/_fdserver.c b/platform/linux-generic/_

> fdserver.c

> index bf36eb2..5cef806 100644

> --- a/platform/linux-generic/_fdserver.c

> +++ b/platform/linux-generic/_fdserver.c

> @@ -73,7 +73,7 @@ typedef struct fdentry_s {

>         uint64_t key;

>         int  fd;

>  } fdentry_t;

> -static fdentry_t fd_table[FDSERVER_MAX_ENTRIES];

> +static fdentry_t *fd_table;

>  static int fd_table_nb_entries;

>

>  /*

> @@ -622,8 +622,20 @@ int _odp_fdserver_init_global(void)

>                 /* TODO: pin the server on appropriate service cpu mask */

>                 /* when (if) we can agree on the usage of service mask  */

>

> +               /* allocate the space for the file descriptor<->key table:

> */

> +               fd_table = malloc(FDSERVER_MAX_ENTRIES *

> sizeof(fdentry_t));

> +               if (!fd_table) {

> +                       ODP_ERR("maloc failed!\n");

> +                       exit(1);

> +               }

> +

> +               /* wait for clients requests */

>                 wait_requests(sock); /* Returns when server is stopped  */

>                 close(sock);

> +

> +               /* release the file descriptor table: */

> +               free(fd_table);

> +

>                 exit(0);

>         }

>

> --

> 2.7.4

>

>
Christophe Milard Sept. 13, 2016, 12:46 p.m. | #2
Hi Bill, and thanks for your review/comments:
I don't think that there any reason (at this time, at least) to worry about
table alignment: The server is -hopefully- used only at non critical times
(such as init...). It is based on socket communications which will be dead
slow compared to the alignment gain anyway.
So I don't think there is much to gain with the alignment.

Christophe.

On 13 September 2016 at 14:02, Bill Fischofer <bill.fischofer@linaro.org>
wrote:

> This looks good now. Only question is whether you need to worry about

> cache-aligning the table, but that's a tuning option that can be considered

> later if needed.

>

> On Tue, Sep 13, 2016 at 7:40 AM, Christophe Milard <

> christophe.milard@linaro.org> wrote:

>

>> The table containing the saved file-descriptors<->{context, key} couples

>> is

>> now dynamically malloc'd in the fd server process, hence avoiding

>> the memory waste which happened in other process when the table was

>> staticaly reserved in all processes.

>>

>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

>>

>

> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

>

>

>> ---

>>  platform/linux-generic/_fdserver.c | 14 +++++++++++++-

>>  1 file changed, 13 insertions(+), 1 deletion(-)

>>

>> diff --git a/platform/linux-generic/_fdserver.c

>> b/platform/linux-generic/_fdserver.c

>> index bf36eb2..5cef806 100644

>> --- a/platform/linux-generic/_fdserver.c

>> +++ b/platform/linux-generic/_fdserver.c

>> @@ -73,7 +73,7 @@ typedef struct fdentry_s {

>>         uint64_t key;

>>         int  fd;

>>  } fdentry_t;

>> -static fdentry_t fd_table[FDSERVER_MAX_ENTRIES];

>> +static fdentry_t *fd_table;

>>  static int fd_table_nb_entries;

>>

>>  /*

>> @@ -622,8 +622,20 @@ int _odp_fdserver_init_global(void)

>>                 /* TODO: pin the server on appropriate service cpu mask */

>>                 /* when (if) we can agree on the usage of service mask  */

>>

>> +               /* allocate the space for the file descriptor<->key

>> table: */

>> +               fd_table = malloc(FDSERVER_MAX_ENTRIES *

>> sizeof(fdentry_t));

>> +               if (!fd_table) {

>> +                       ODP_ERR("maloc failed!\n");

>> +                       exit(1);

>> +               }

>> +

>> +               /* wait for clients requests */

>>                 wait_requests(sock); /* Returns when server is stopped  */

>>                 close(sock);

>> +

>> +               /* release the file descriptor table: */

>> +               free(fd_table);

>> +

>>                 exit(0);

>>         }

>>

>> --

>> 2.7.4

>>

>>

>
Maxim Uvarov Sept. 14, 2016, 2:51 p.m. | #3
Merged,
Maxim.

On 09/13/16 15:46, Christophe Milard wrote:
> Hi Bill, and thanks for your review/comments:

> I don't think that there any reason (at this time, at least) to worry about

> table alignment: The server is -hopefully- used only at non critical times

> (such as init...). It is based on socket communications which will be dead

> slow compared to the alignment gain anyway.

> So I don't think there is much to gain with the alignment.

>

> Christophe.

>

> On 13 September 2016 at 14:02, Bill Fischofer <bill.fischofer@linaro.org>

> wrote:

>

>> This looks good now. Only question is whether you need to worry about

>> cache-aligning the table, but that's a tuning option that can be considered

>> later if needed.

>>

>> On Tue, Sep 13, 2016 at 7:40 AM, Christophe Milard <

>> christophe.milard@linaro.org> wrote:

>>

>>> The table containing the saved file-descriptors<->{context, key} couples

>>> is

>>> now dynamically malloc'd in the fd server process, hence avoiding

>>> the memory waste which happened in other process when the table was

>>> staticaly reserved in all processes.

>>>

>>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

>>>

>> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

>>

>>

>>> ---

>>>   platform/linux-generic/_fdserver.c | 14 +++++++++++++-

>>>   1 file changed, 13 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/platform/linux-generic/_fdserver.c

>>> b/platform/linux-generic/_fdserver.c

>>> index bf36eb2..5cef806 100644

>>> --- a/platform/linux-generic/_fdserver.c

>>> +++ b/platform/linux-generic/_fdserver.c

>>> @@ -73,7 +73,7 @@ typedef struct fdentry_s {

>>>          uint64_t key;

>>>          int  fd;

>>>   } fdentry_t;

>>> -static fdentry_t fd_table[FDSERVER_MAX_ENTRIES];

>>> +static fdentry_t *fd_table;

>>>   static int fd_table_nb_entries;

>>>

>>>   /*

>>> @@ -622,8 +622,20 @@ int _odp_fdserver_init_global(void)

>>>                  /* TODO: pin the server on appropriate service cpu mask */

>>>                  /* when (if) we can agree on the usage of service mask  */

>>>

>>> +               /* allocate the space for the file descriptor<->key

>>> table: */

>>> +               fd_table = malloc(FDSERVER_MAX_ENTRIES *

>>> sizeof(fdentry_t));

>>> +               if (!fd_table) {

>>> +                       ODP_ERR("maloc failed!\n");

>>> +                       exit(1);

>>> +               }

>>> +

>>> +               /* wait for clients requests */

>>>                  wait_requests(sock); /* Returns when server is stopped  */

>>>                  close(sock);

>>> +

>>> +               /* release the file descriptor table: */

>>> +               free(fd_table);

>>> +

>>>                  exit(0);

>>>          }

>>>

>>> --

>>> 2.7.4

>>>

>>>

Patch

diff --git a/platform/linux-generic/_fdserver.c b/platform/linux-generic/_fdserver.c
index bf36eb2..5cef806 100644
--- a/platform/linux-generic/_fdserver.c
+++ b/platform/linux-generic/_fdserver.c
@@ -73,7 +73,7 @@  typedef struct fdentry_s {
 	uint64_t key;
 	int  fd;
 } fdentry_t;
-static fdentry_t fd_table[FDSERVER_MAX_ENTRIES];
+static fdentry_t *fd_table;
 static int fd_table_nb_entries;
 
 /*
@@ -622,8 +622,20 @@  int _odp_fdserver_init_global(void)
 		/* TODO: pin the server on appropriate service cpu mask */
 		/* when (if) we can agree on the usage of service mask  */
 
+		/* allocate the space for the file descriptor<->key table: */
+		fd_table = malloc(FDSERVER_MAX_ENTRIES * sizeof(fdentry_t));
+		if (!fd_table) {
+			ODP_ERR("maloc failed!\n");
+			exit(1);
+		}
+
+		/* wait for clients requests */
 		wait_requests(sock); /* Returns when server is stopped  */
 		close(sock);
+
+		/* release the file descriptor table: */
+		free(fd_table);
+
 		exit(0);
 	}