[v2] Clean up check_pf allocation pattern. addresses
Commit Message
On Wed, Mar 26, 2014 at 10:53:17AM +0100, Andreas Schwab wrote:
> Ondřej Bílka <neleai@seznam.cz> writes:
>
> > diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
> > index e6a12ed..40b1af2 100644
> > --- a/sysdeps/unix/sysv/linux/check_pf.c
> > +++ b/sysdeps/unix/sysv/linux/check_pf.c
> > @@ -106,6 +106,12 @@ cache_valid_p (void)
> > static struct cached_data *
> > make_request (int fd, pid_t pid)
> > {
> > +
>
> Extra empty line.
>
> > @@ -241,40 +230,36 @@ make_request (int fd, pid_t pid)
> > }
> > }
> >
> > - struct in6ailist *newp;
> > - if (__libc_use_alloca (alloca_used + sizeof (*newp)))
> > + if (result_len == 0 || result_len == result_cap)
> > {
> > - newp = alloca_account (sizeof (*newp), alloca_used);
> > - newp->use_malloc = false;
> > + result_cap = 2 * result_cap;
> > + result = realloc (result, sizeof (*result)
> > + + result_cap * sizeof (struct in6addrinfo));
>
> Wrong indent.
>
> > + info->flags = (((ifam->ifa_flags
> > & (IFA_F_DEPRECATED
>
> Wrong indent.
>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
OK, here is v2
* sysdeps/unix/sysv/linux/check_pf.c (make_pf): Simplify
allocation strategy.
Comments
Ondřej Bílka <neleai@seznam.cz> writes:
> @@ -137,20 +142,9 @@ make_request (int fd, pid_t pid)
> #else
> const size_t buf_size = __getpagesize ();
> #endif
> - bool use_malloc = false;
> char *buf;
> - size_t alloca_used = 0;
>
> - if (__libc_use_alloca (buf_size))
> - buf = alloca_account (buf_size, alloca_used);
> - else
> - {
> - buf = malloc (buf_size);
> - if (buf != NULL)
> - use_malloc = true;
> - else
> - goto out_fail;
> - }
> + buf = alloca (buf_size);
Does the compiler optimize this into a plain array if buf_size is
constant?
Andreas.
On Wed, May 28, 2014 at 12:36:12PM +0200, Andreas Schwab wrote:
> Ondřej Bílka <neleai@seznam.cz> writes:
>
> > @@ -137,20 +142,9 @@ make_request (int fd, pid_t pid)
> > #else
> > const size_t buf_size = __getpagesize ();
> > #endif
> > - bool use_malloc = false;
> > char *buf;
> > - size_t alloca_used = 0;
> >
> > - if (__libc_use_alloca (buf_size))
> > - buf = alloca_account (buf_size, alloca_used);
> > - else
> > - {
> > - buf = malloc (buf_size);
> > - if (buf != NULL)
> > - use_malloc = true;
> > - else
> > - goto out_fail;
> > - }
> > + buf = alloca (buf_size);
>
> Does the compiler optimize this into a plain array if buf_size is
> constant?
>
No, that would not be problem if gcc optimized alloca alignment which it
does not do now.
Ondřej Bílka <neleai@seznam.cz> writes:
> No, that would not be problem if gcc optimized alloca alignment which it
> does not do now.
What do you mean with "alloca alignment"?
Andreas.
On Wed, May 28, 2014 at 01:02:10PM +0200, Andreas Schwab wrote:
> Ondřej Bílka <neleai@seznam.cz> writes:
>
> > No, that would not be problem if gcc optimized alloca alignment which it
> > does not do now.
>
> What do you mean with "alloca alignment"?
>
How that does expand in assembly, you need to align pointers to 16
bytes. A following fragment
int main()
{
char *x = alloca (42);
foo (x);
}
with stack pointer named sp growing downward gets expanded to
int main()
{
register char *tmp = sp;
sp -= 64;
char *x = (sp + 15) & (-16);
foo (x);
sp = tmp;
}
but when there is array
int main()
{
char *x[42];
foo (x);
}
then code becomes
int main()
{
sp -= 56;
x = sp;
foo (x);
sp += 56;
}
One more reason not to use alloca here.
Andreas.
@@ -106,6 +106,11 @@ cache_valid_p (void)
static struct cached_data *
make_request (int fd, pid_t pid)
{
+ struct cached_data *result = NULL;
+
+ size_t result_len = 0;
+ size_t result_cap = 32;
+
struct req
{
struct nlmsghdr nlh;
@@ -137,20 +142,9 @@ make_request (int fd, pid_t pid)
#else
const size_t buf_size = __getpagesize ();
#endif
- bool use_malloc = false;
char *buf;
- size_t alloca_used = 0;
- if (__libc_use_alloca (buf_size))
- buf = alloca_account (buf_size, alloca_used);
- else
- {
- buf = malloc (buf_size);
- if (buf != NULL)
- use_malloc = true;
- else
- goto out_fail;
- }
+ buf = alloca (buf_size);
struct iovec iov = { buf, buf_size };
@@ -160,13 +154,7 @@ make_request (int fd, pid_t pid)
goto out_fail;
bool done = false;
- struct in6ailist
- {
- struct in6addrinfo info;
- struct in6ailist *next;
- bool use_malloc;
- } *in6ailist = NULL;
- size_t in6ailistlen = 0;
+
bool seen_ipv4 = false;
bool seen_ipv6 = false;
@@ -182,10 +170,10 @@ make_request (int fd, pid_t pid)
ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
if (read_len < 0)
- goto out_fail2;
+ goto out_fail;
if (msg.msg_flags & MSG_TRUNC)
- goto out_fail2;
+ goto out_fail;
struct nlmsghdr *nlmh;
for (nlmh = (struct nlmsghdr *) buf;
@@ -241,40 +229,35 @@ make_request (int fd, pid_t pid)
}
}
- struct in6ailist *newp;
- if (__libc_use_alloca (alloca_used + sizeof (*newp)))
- {
- newp = alloca_account (sizeof (*newp), alloca_used);
- newp->use_malloc = false;
- }
- else
+ if (result_len == 0 || result_len == result_cap)
{
- newp = malloc (sizeof (*newp));
- if (newp == NULL)
- goto out_fail2;
- newp->use_malloc = true;
+ result_cap = 2 * result_cap;
+ result = realloc (result, sizeof (*result)
+ + result_cap
+ * sizeof (struct in6addrinfo));
}
- newp->info.flags = (((ifam->ifa_flags
- & (IFA_F_DEPRECATED
- | IFA_F_OPTIMISTIC))
- ? in6ai_deprecated : 0)
- | ((ifam->ifa_flags
- & IFA_F_HOMEADDRESS)
- ? in6ai_homeaddress : 0));
- newp->info.prefixlen = ifam->ifa_prefixlen;
- newp->info.index = ifam->ifa_index;
+
+ if (!result)
+ goto out_fail;
+
+ struct in6addrinfo *info = &result->in6ai[result_len++];
+
+ info->flags = (((ifam->ifa_flags
+ & (IFA_F_DEPRECATED | IFA_F_OPTIMISTIC))
+ ? in6ai_deprecated : 0)
+ | ((ifam->ifa_flags & IFA_F_HOMEADDRESS)
+ ? in6ai_homeaddress : 0));
+ info->prefixlen = ifam->ifa_prefixlen;
+ info->index = ifam->ifa_index;
if (ifam->ifa_family == AF_INET)
{
- newp->info.addr[0] = 0;
- newp->info.addr[1] = 0;
- newp->info.addr[2] = htonl (0xffff);
- newp->info.addr[3] = *(const in_addr_t *) address;
+ info->addr[0] = 0;
+ info->addr[1] = 0;
+ info->addr[2] = htonl (0xffff);
+ info->addr[3] = *(const in_addr_t *) address;
}
else
- memcpy (newp->info.addr, address, sizeof (newp->info.addr));
- newp->next = in6ailist;
- in6ailist = newp;
- ++in6ailistlen;
+ memcpy (info->addr, address, sizeof (info->addr));
}
else if (nlmh->nlmsg_type == NLMSG_DONE)
/* We found the end, leave the loop. */
@@ -283,53 +266,29 @@ make_request (int fd, pid_t pid)
}
while (! done);
- struct cached_data *result;
- if (seen_ipv6 && in6ailist != NULL)
+ if (seen_ipv6 && result != NULL)
{
- result = malloc (sizeof (*result)
- + in6ailistlen * sizeof (struct in6addrinfo));
- if (result == NULL)
- goto out_fail2;
-
result->timestamp = get_nl_timestamp ();
result->usecnt = 2;
result->seen_ipv4 = seen_ipv4;
result->seen_ipv6 = true;
- result->in6ailen = in6ailistlen;
-
- do
- {
- result->in6ai[--in6ailistlen] = in6ailist->info;
- struct in6ailist *next = in6ailist->next;
- if (in6ailist->use_malloc)
- free (in6ailist);
- in6ailist = next;
- }
- while (in6ailist != NULL);
+ result->in6ailen = result_len;
}
else
{
+ free (result);
+
atomic_add (&noai6ai_cached.usecnt, 2);
noai6ai_cached.seen_ipv4 = seen_ipv4;
noai6ai_cached.seen_ipv6 = seen_ipv6;
result = &noai6ai_cached;
}
- if (use_malloc)
- free (buf);
return result;
- out_fail2:
- while (in6ailist != NULL)
- {
- struct in6ailist *next = in6ailist->next;
- if (in6ailist->use_malloc)
- free (in6ailist);
- in6ailist = next;
- }
out_fail:
- if (use_malloc)
- free (buf);
+
+ free (result);
return NULL;
}