| Message ID | 20171107200512.3715-1-rosenp@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [LEDE-DEV] procd: Remove unnecessary memset calls. | expand |
Both memset() and calloc() have highly optimised implementations, so the expected gains with this patch for the allocation of zeroed memory will be small at best. As this patch does not fix a bug: why is the change "needed" ? Just curiosity, bye, Paul > Op 7 nov. 2017, om 21:05 heeft Rosen Penev <rosenp@gmail.com> het volgende geschreven: > > Changes allocation to calloc and {} as needed. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > inittab.c | 6 ++---- > plug/hotplug.c | 7 ++----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/inittab.c b/inittab.c > index 21172f7..c27c324 100644 > --- a/inittab.c > +++ b/inittab.c > @@ -284,8 +284,7 @@ void procd_inittab(void) > > regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA-Z0-9]*):(.*)", REG_EXTENDED); > line = malloc(LINE_LEN); > - a = malloc(sizeof(struct init_action)); > - memset(a, 0, sizeof(struct init_action)); > + a = calloc(1, sizeof(struct init_action)); > > while (fgets(line, LINE_LEN, fp)) { > char *tags[TAG_PROCESS + 1]; > @@ -322,8 +321,7 @@ void procd_inittab(void) > if (add_action(a, tags[TAG_ACTION])) > continue; > line = malloc(LINE_LEN); > - a = malloc(sizeof(struct init_action)); > - memset(a, 0, sizeof(struct init_action)); > + a = calloc(1, sizeof(struct init_action)); > } > > fclose(fp); > diff --git a/plug/hotplug.c b/plug/hotplug.c > index 49c177f..6e55f67 100644 > --- a/plug/hotplug.c > +++ b/plug/hotplug.c > @@ -434,12 +434,10 @@ static void handle_button_complete(struct blob_attr *msg, struct blob_attr *data > if (!name) > return; > > - b = malloc(sizeof(*b)); > + b = calloc(1, sizeof(*b)); > if (!b) > return; > > - memset(b, 0, sizeof(*b)); > - > b->data = malloc(blob_pad_len(data)); > b->name = strdup(name); > b->seen = timeout; > @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler handler) > > void hotplug(char *rules) > { > - struct sockaddr_nl nls; > + struct sockaddr_nl nls = {}; > int nlbufsize = 512 * 1024; > > rule_file = strdup(rules); > - memset(&nls,0,sizeof(struct sockaddr_nl)); > nls.nl_family = AF_NETLINK; > nls.nl_pid = getpid(); > nls.nl_groups = -1; > -- > 2.13.6 > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
On Wed, 2017-11-08 at 11:57 +0100, Paul Oranje wrote: > Both memset() and calloc() have highly optimised implementations, so > the expected gains with this patch for the allocation of zeroed > memory will be small at best. As this patch does not fix a bug: why > is the change "needed" ? > Style changes are strictly speaking not "needed". > Just curiosity, bye, > Paul > > > Op 7 nov. 2017, om 21:05 heeft Rosen Penev <rosenp@gmail.com> het > > volgende geschreven: > > > > Changes allocation to calloc and {} as needed. > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > --- > > inittab.c | 6 ++---- > > plug/hotplug.c | 7 ++----- > > 2 files changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/inittab.c b/inittab.c > > index 21172f7..c27c324 100644 > > --- a/inittab.c > > +++ b/inittab.c > > @@ -284,8 +284,7 @@ void procd_inittab(void) > > > > regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA-Z0- > > 9]*):(.*)", REG_EXTENDED); > > line = malloc(LINE_LEN); > > - a = malloc(sizeof(struct init_action)); > > - memset(a, 0, sizeof(struct init_action)); > > + a = calloc(1, sizeof(struct init_action)); > > > > while (fgets(line, LINE_LEN, fp)) { > > char *tags[TAG_PROCESS + 1]; > > @@ -322,8 +321,7 @@ void procd_inittab(void) > > if (add_action(a, tags[TAG_ACTION])) > > continue; > > line = malloc(LINE_LEN); > > - a = malloc(sizeof(struct init_action)); > > - memset(a, 0, sizeof(struct init_action)); > > + a = calloc(1, sizeof(struct init_action)); > > } > > > > fclose(fp); > > diff --git a/plug/hotplug.c b/plug/hotplug.c > > index 49c177f..6e55f67 100644 > > --- a/plug/hotplug.c > > +++ b/plug/hotplug.c > > @@ -434,12 +434,10 @@ static void handle_button_complete(struct > > blob_attr *msg, struct blob_attr *data > > if (!name) > > return; > > > > - b = malloc(sizeof(*b)); > > + b = calloc(1, sizeof(*b)); > > if (!b) > > return; > > > > - memset(b, 0, sizeof(*b)); > > - > > b->data = malloc(blob_pad_len(data)); > > b->name = strdup(name); > > b->seen = timeout; > > @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler > > handler) > > > > void hotplug(char *rules) > > { > > - struct sockaddr_nl nls; > > + struct sockaddr_nl nls = {}; > > int nlbufsize = 512 * 1024; > > > > rule_file = strdup(rules); > > - memset(&nls,0,sizeof(struct sockaddr_nl)); > > nls.nl_family = AF_NETLINK; > > nls.nl_pid = getpid(); > > nls.nl_groups = -1; > > -- > > 2.13.6 > > > > > > _______________________________________________ > > Lede-dev mailing list > > Lede-dev@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/lede-dev > >
Thanks, also for the link (https://vorpus.org/blog/why-does-calloc-exist/). This article made me aware that using calloc() may be wise. For security reasons: calloc avoids possible arrhythmic overflows when the allocation size for malloc is the product of two numbers. And because it may offer real performance advantages with big allocations: calloc() can make good use of the VM syscalls, whereas memset() needs to initialise all allocated memory at once which requires that concerned memory is paged in, with calloc() memory is allocated sparsely. The article makes a good argument for using malloc(), but still one should agree with Arjen de Korte, that no changes should be made without necessity. When I was an active C programmer I was called "de snoeier" (Dutch for pruner) and did, to mine and others dismay, introduce errors. Not making errors, even with seemingly trivial changes, is hard. For new code this disadvantage of changing working code is absent ... Paul > Op 8 nov. 2017, om 19:59 heeft rosenp@gmail.com het volgende geschreven: > > On Wed, 2017-11-08 at 11:57 +0100, Paul Oranje wrote: >> Both memset() and calloc() have highly optimised implementations, so >> the expected gains with this patch for the allocation of zeroed >> memory will be small at best. As this patch does not fix a bug: why >> is the change "needed" ? >> > Style changes are strictly speaking not "needed". >> Just curiosity, bye, >> Paul >> >>> Op 7 nov. 2017, om 21:05 heeft Rosen Penev <rosenp@gmail.com> het >>> volgende geschreven: >>> >>> Changes allocation to calloc and {} as needed. >>> >>> Signed-off-by: Rosen Penev <rosenp@gmail.com> >>> --- >>> inittab.c | 6 ++---- >>> plug/hotplug.c | 7 ++----- >>> 2 files changed, 4 insertions(+), 9 deletions(-) >>> >>> diff --git a/inittab.c b/inittab.c >>> index 21172f7..c27c324 100644 >>> --- a/inittab.c >>> +++ b/inittab.c >>> @@ -284,8 +284,7 @@ void procd_inittab(void) >>> >>> regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA-Z0- >>> 9]*):(.*)", REG_EXTENDED); >>> line = malloc(LINE_LEN); >>> - a = malloc(sizeof(struct init_action)); >>> - memset(a, 0, sizeof(struct init_action)); >>> + a = calloc(1, sizeof(struct init_action)); >>> >>> while (fgets(line, LINE_LEN, fp)) { >>> char *tags[TAG_PROCESS + 1]; >>> @@ -322,8 +321,7 @@ void procd_inittab(void) >>> if (add_action(a, tags[TAG_ACTION])) >>> continue; >>> line = malloc(LINE_LEN); >>> - a = malloc(sizeof(struct init_action)); >>> - memset(a, 0, sizeof(struct init_action)); >>> + a = calloc(1, sizeof(struct init_action)); >>> } >>> >>> fclose(fp); >>> diff --git a/plug/hotplug.c b/plug/hotplug.c >>> index 49c177f..6e55f67 100644 >>> --- a/plug/hotplug.c >>> +++ b/plug/hotplug.c >>> @@ -434,12 +434,10 @@ static void handle_button_complete(struct >>> blob_attr *msg, struct blob_attr *data >>> if (!name) >>> return; >>> >>> - b = malloc(sizeof(*b)); >>> + b = calloc(1, sizeof(*b)); >>> if (!b) >>> return; >>> >>> - memset(b, 0, sizeof(*b)); >>> - >>> b->data = malloc(blob_pad_len(data)); >>> b->name = strdup(name); >>> b->seen = timeout; >>> @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler >>> handler) >>> >>> void hotplug(char *rules) >>> { >>> - struct sockaddr_nl nls; >>> + struct sockaddr_nl nls = {}; >>> int nlbufsize = 512 * 1024; >>> >>> rule_file = strdup(rules); >>> - memset(&nls,0,sizeof(struct sockaddr_nl)); >>> nls.nl_family = AF_NETLINK; >>> nls.nl_pid = getpid(); >>> nls.nl_groups = -1; >>> -- >>> 2.13.6 >>> >>> >>> _______________________________________________ >>> Lede-dev mailing list >>> Lede-dev@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/lede-dev >> >>
Tested compile size difference. Saves 32 bytes. ¯\_(ツ)_/¯ On Tue, 2017-11-07 at 12:05 -0800, Rosen Penev wrote: > Changes allocation to calloc and {} as needed. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > inittab.c | 6 ++---- > plug/hotplug.c | 7 ++----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/inittab.c b/inittab.c > index 21172f7..c27c324 100644 > --- a/inittab.c > +++ b/inittab.c > @@ -284,8 +284,7 @@ void procd_inittab(void) > > regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA- > Z0-9]*):(.*)", REG_EXTENDED); > line = malloc(LINE_LEN); > - a = malloc(sizeof(struct init_action)); > - memset(a, 0, sizeof(struct init_action)); > + a = calloc(1, sizeof(struct init_action)); > > while (fgets(line, LINE_LEN, fp)) { > char *tags[TAG_PROCESS + 1]; > @@ -322,8 +321,7 @@ void procd_inittab(void) > if (add_action(a, tags[TAG_ACTION])) > continue; > line = malloc(LINE_LEN); > - a = malloc(sizeof(struct init_action)); > - memset(a, 0, sizeof(struct init_action)); > + a = calloc(1, sizeof(struct init_action)); > } > > fclose(fp); > diff --git a/plug/hotplug.c b/plug/hotplug.c > index 49c177f..6e55f67 100644 > --- a/plug/hotplug.c > +++ b/plug/hotplug.c > @@ -434,12 +434,10 @@ static void handle_button_complete(struct > blob_attr *msg, struct blob_attr *data > if (!name) > return; > > - b = malloc(sizeof(*b)); > + b = calloc(1, sizeof(*b)); > if (!b) > return; > > - memset(b, 0, sizeof(*b)); > - > b->data = malloc(blob_pad_len(data)); > b->name = strdup(name); > b->seen = timeout; > @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler > handler) > > void hotplug(char *rules) > { > - struct sockaddr_nl nls; > + struct sockaddr_nl nls = {}; > int nlbufsize = 512 * 1024; > > rule_file = strdup(rules); > - memset(&nls,0,sizeof(struct sockaddr_nl)); > nls.nl_family = AF_NETLINK; > nls.nl_pid = getpid(); > nls.nl_groups = -1;
On 11/12/2017 10:53 PM, rosenp@gmail.com wrote: > Tested compile size difference. Saves 32 bytes. ¯\_(ツ)_/¯ > > On Tue, 2017-11-07 at 12:05 -0800, Rosen Penev wrote: >> Changes allocation to calloc and {} as needed. >> >> Signed-off-by: Rosen Penev <rosenp@gmail.com> >> --- >> inittab.c | 6 ++---- >> plug/hotplug.c | 7 ++----- >> 2 files changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/inittab.c b/inittab.c >> index 21172f7..c27c324 100644 >> --- a/inittab.c >> +++ b/inittab.c >> @@ -284,8 +284,7 @@ void procd_inittab(void) >> >> regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA- >> Z0-9]*):(.*)", REG_EXTENDED); >> line = malloc(LINE_LEN); >> - a = malloc(sizeof(struct init_action)); >> - memset(a, 0, sizeof(struct init_action)); >> + a = calloc(1, sizeof(struct init_action)); >> >> while (fgets(line, LINE_LEN, fp)) { >> char *tags[TAG_PROCESS + 1]; >> @@ -322,8 +321,7 @@ void procd_inittab(void) >> if (add_action(a, tags[TAG_ACTION])) >> continue; >> line = malloc(LINE_LEN); >> - a = malloc(sizeof(struct init_action)); >> - memset(a, 0, sizeof(struct init_action)); >> + a = calloc(1, sizeof(struct init_action)); >> } >> >> fclose(fp); >> diff --git a/plug/hotplug.c b/plug/hotplug.c >> index 49c177f..6e55f67 100644 >> --- a/plug/hotplug.c >> +++ b/plug/hotplug.c >> @@ -434,12 +434,10 @@ static void handle_button_complete(struct >> blob_attr *msg, struct blob_attr *data >> if (!name) >> return; >> >> - b = malloc(sizeof(*b)); >> + b = calloc(1, sizeof(*b)); >> if (!b) >> return; >> >> - memset(b, 0, sizeof(*b)); >> - >> b->data = malloc(blob_pad_len(data)); >> b->name = strdup(name); >> b->seen = timeout; >> @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler >> handler) >> >> void hotplug(char *rules) >> { >> - struct sockaddr_nl nls; >> + struct sockaddr_nl nls = {}; >> int nlbufsize = 512 * 1024; >> >> rule_file = strdup(rules); >> - memset(&nls,0,sizeof(struct sockaddr_nl)); >> nls.nl_family = AF_NETLINK; >> nls.nl_pid = getpid(); >> nls.nl_groups = -1; > wrt. hotplug() -- When a fixed size aggregate will live only on the stack, initializing it is faster than calling allocation function(s). Not that any _one_ of them would be significant, but as a coding pattern it can add up. Even if you permit in-line standard functions, then there still are presumed behaviors of the function which will likely be implemented. Initialization on the stack will not incur the overhead. - Eric
diff --git a/inittab.c b/inittab.c index 21172f7..c27c324 100644 --- a/inittab.c +++ b/inittab.c @@ -284,8 +284,7 @@ void procd_inittab(void) regcomp(&pat_inittab, "([a-zA-Z0-9]*):([a-zA-Z0-9]*):([a-zA-Z0-9]*):(.*)", REG_EXTENDED); line = malloc(LINE_LEN); - a = malloc(sizeof(struct init_action)); - memset(a, 0, sizeof(struct init_action)); + a = calloc(1, sizeof(struct init_action)); while (fgets(line, LINE_LEN, fp)) { char *tags[TAG_PROCESS + 1]; @@ -322,8 +321,7 @@ void procd_inittab(void) if (add_action(a, tags[TAG_ACTION])) continue; line = malloc(LINE_LEN); - a = malloc(sizeof(struct init_action)); - memset(a, 0, sizeof(struct init_action)); + a = calloc(1, sizeof(struct init_action)); } fclose(fp); diff --git a/plug/hotplug.c b/plug/hotplug.c index 49c177f..6e55f67 100644 --- a/plug/hotplug.c +++ b/plug/hotplug.c @@ -434,12 +434,10 @@ static void handle_button_complete(struct blob_attr *msg, struct blob_attr *data if (!name) return; - b = malloc(sizeof(*b)); + b = calloc(1, sizeof(*b)); if (!b) return; - memset(b, 0, sizeof(*b)); - b->data = malloc(blob_pad_len(data)); b->name = strdup(name); b->seen = timeout; @@ -584,11 +582,10 @@ void hotplug_last_event(uloop_timeout_handler handler) void hotplug(char *rules) { - struct sockaddr_nl nls; + struct sockaddr_nl nls = {}; int nlbufsize = 512 * 1024; rule_file = strdup(rules); - memset(&nls,0,sizeof(struct sockaddr_nl)); nls.nl_family = AF_NETLINK; nls.nl_pid = getpid(); nls.nl_groups = -1;
Changes allocation to calloc and {} as needed. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- inittab.c | 6 ++---- plug/hotplug.c | 7 ++----- 2 files changed, 4 insertions(+), 9 deletions(-)