diff mbox

[v2] Clean up check_pf allocation pattern. addresses

Message ID 20140527205640.GB21440@domone.podge
State New
Headers show

Commit Message

Ondřej Bílka May 27, 2014, 8:56 p.m. UTC 
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

Andreas Schwab May 28, 2014, 10:36 a.m. UTC | #1 
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.
Ondřej Bílka May 28, 2014, 10:58 a.m. UTC | #2 
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.
Andreas Schwab May 28, 2014, 11:02 a.m. UTC | #3 
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.
Ondřej Bílka June 5, 2014, 11:51 a.m. UTC | #4 
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;
}
Andreas Schwab June 5, 2014, 12:31 p.m. UTC | #5 
One more reason not to use alloca here.

Andreas.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
index e6a12ed..2c053d0 100644
--- a/sysdeps/unix/sysv/linux/check_pf.c
+++ b/sysdeps/unix/sysv/linux/check_pf.c
@@ -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;
 }