diff mbox

[02/25] pthread_setaffinity (Linux variant): Rewrite to use VLA instead of alloca

Message ID 5506E8E6.1000906@redhat.com
State New
Headers show

Commit Message

Florian Weimer March 16, 2015, 2:29 p.m. UTC 
On 03/02/2015 08:58 PM, Mike Frysinger wrote:
> On 01 Mar 2015 15:05, Florian Weimer wrote:
>> extend_alloca was used to emulate VLA deallocation.
> 
> general note: i found the old code easier to read.  i think GNU style requires 
> newlines after decls and before the return statement.  that would certainly help  
> readability here.

What about this version?  I think it makes the control flow more
explicit, and it prompted me to deal differently with the res == 0 case
(which was swept under the carpet before).

2015-03-16  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/unix/sysv/linux/pthread_setaffinity.c
	(__determine_cpumask_size): Replace extend_alloca with a
	variable-length array.  Always return zero if res == 0.

Comments

Andreas Schwab March 16, 2015, 2:49 p.m. UTC | #1 
Florian Weimer <fweimer@redhat.com> writes:

> @@ -27,26 +26,29 @@
>  size_t __kernel_cpumask_size attribute_hidden;
>  
>  
> -/* Determine the current affinity.  As a side affect we learn
> -   about the size of the cpumask_t in the kernel.  */
> +/* Determine the size of cpumask_t in the kernel.  */
>  int
>  __determine_cpumask_size (pid_t tid)
>  {
> -  INTERNAL_SYSCALL_DECL (err);
> -  int res;
> -
>    size_t psize = 128;
> -  void *p = alloca (psize);
> -
> -  while (res = INTERNAL_SYSCALL (sched_getaffinity, err, 3, tid, psize, p),
> -	 INTERNAL_SYSCALL_ERROR_P (res, err)
> -	 && INTERNAL_SYSCALL_ERRNO (res, err) == EINVAL)
> -    p = extend_alloca (p, psize, 2 * psize);
> +  int res;
>  
> -  if (res == 0 || INTERNAL_SYSCALL_ERROR_P (res, err))
> -    return INTERNAL_SYSCALL_ERRNO (res, err);
> +  for (psize = 128; ; psize *= 2)

The assignment is redundant.

Andreas.
Florian Weimer March 16, 2015, 2:49 p.m. UTC | #2 
On 03/16/2015 03:49 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> @@ -27,26 +26,29 @@
>>  size_t __kernel_cpumask_size attribute_hidden;
>>  
>>  
>> -/* Determine the current affinity.  As a side affect we learn
>> -   about the size of the cpumask_t in the kernel.  */
>> +/* Determine the size of cpumask_t in the kernel.  */
>>  int
>>  __determine_cpumask_size (pid_t tid)
>>  {
>> -  INTERNAL_SYSCALL_DECL (err);
>> -  int res;
>> -
>>    size_t psize = 128;
>> -  void *p = alloca (psize);
>> -
>> -  while (res = INTERNAL_SYSCALL (sched_getaffinity, err, 3, tid, psize, p),
>> -	 INTERNAL_SYSCALL_ERROR_P (res, err)
>> -	 && INTERNAL_SYSCALL_ERRNO (res, err) == EINVAL)
>> -    p = extend_alloca (p, psize, 2 * psize);
>> +  int res;
>>  
>> -  if (res == 0 || INTERNAL_SYSCALL_ERROR_P (res, err))
>> -    return INTERNAL_SYSCALL_ERRNO (res, err);
>> +  for (psize = 128; ; psize *= 2)
> 
> The assignment is redundant.

It is; I'll drop it from the initialization.
Mike Frysinger March 16, 2015, 4:51 p.m. UTC | #3 
On 16 Mar 2015 15:29, Florian Weimer wrote:
> +  for (psize = 128; ; psize *= 2)
> +    {
> +      char buf[psize];
> +      INTERNAL_SYSCALL_DECL (err);

style says there should be a blank line here -- after the decls and before the 
first line of code (the res assignment).  otherwise, this does look better than 
v1.  thanks!
-mike
Florian Weimer March 23, 2015, 3:42 p.m. UTC | #4 
On 03/16/2015 05:51 PM, Mike Frysinger wrote:
> On 16 Mar 2015 15:29, Florian Weimer wrote:
>> +  for (psize = 128; ; psize *= 2) +    { +      char
>> buf[psize]; +      INTERNAL_SYSCALL_DECL (err);
> 
> style says there should be a blank line here -- after the decls and
> before the first line of code (the res assignment).  otherwise,
> this does look better than v1.  thanks!

Thanks, I've committed it after adding the blank line.

Could you have a look at the sister patch as well?  Thanks.

<https://sourceware.org/ml/libc-alpha/2015-03/msg00544.html>
diff mbox

Patch

From 5dddb59c0c8ea06afebfee70026b854fbaf487f1 Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Sun, 1 Mar 2015 15:05:09 +0100
Subject: [PATCH] pthread_setaffinity (Linux variant): Rewrite to use VLA
 instead of alloca

extend_alloca was used to emulate VLA deallocation.  The new version
also handles the res == 0 corner case more explicitly, by returning 0
instead of the (potentially undefined, but usually zero) system call
error.
---
 sysdeps/unix/sysv/linux/pthread_setaffinity.c | 32 ++++++++++++++-------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/pthread_setaffinity.c b/sysdeps/unix/sysv/linux/pthread_setaffinity.c
index 37997e9..9137e56 100644
--- a/sysdeps/unix/sysv/linux/pthread_setaffinity.c
+++ b/sysdeps/unix/sysv/linux/pthread_setaffinity.c
@@ -16,7 +16,6 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <errno.h>
 #include <pthreadP.h>
 #include <sysdep.h>
@@ -27,26 +26,29 @@ 
 size_t __kernel_cpumask_size attribute_hidden;
 
 
-/* Determine the current affinity.  As a side affect we learn
-   about the size of the cpumask_t in the kernel.  */
+/* Determine the size of cpumask_t in the kernel.  */
 int
 __determine_cpumask_size (pid_t tid)
 {
-  INTERNAL_SYSCALL_DECL (err);
-  int res;
-
   size_t psize = 128;
-  void *p = alloca (psize);
-
-  while (res = INTERNAL_SYSCALL (sched_getaffinity, err, 3, tid, psize, p),
-	 INTERNAL_SYSCALL_ERROR_P (res, err)
-	 && INTERNAL_SYSCALL_ERRNO (res, err) == EINVAL)
-    p = extend_alloca (p, psize, 2 * psize);
+  int res;
 
-  if (res == 0 || INTERNAL_SYSCALL_ERROR_P (res, err))
-    return INTERNAL_SYSCALL_ERRNO (res, err);
+  for (psize = 128; ; psize *= 2)
+    {
+      char buf[psize];
+      INTERNAL_SYSCALL_DECL (err);
+      res = INTERNAL_SYSCALL (sched_getaffinity, err, 3, tid, psize, buf);
+      if (INTERNAL_SYSCALL_ERROR_P (res, err))
+	{
+	  if (INTERNAL_SYSCALL_ERRNO (res, err) != EINVAL)
+	    return INTERNAL_SYSCALL_ERRNO (res, err);
+	}
+      else
+	break;
+    }
 
-  __kernel_cpumask_size = res;
+  if (res != 0)
+    __kernel_cpumask_size = res;
 
   return 0;
 }
-- 
2.1.0