Re: [PATCH v4] Make bindresvport() function to multithread-safe

[Peng Haitao <penght at cn dot fujitsu dot com>]

On 09/28/2012 10:27 PM, Carlos O'Donell wrote:
>>  /*
>>   * Bind a socket to a privileged IP port
>>   */
>>  int
>>  bindresvport (int sd, struct sockaddr_in *sin)
>>  {
>> -  static short port;
>> +  static __thread short port;
> 
> I'm curious how our relocation count changes with this?
> 
> Net neutral or did we gain extra relocations by changing this from
> static to static __thread?
> 
> Could you have a look into that?
> 

The static variable will cause bindresvport to unsafe.

Use thread A and thread B to explain:

thread A: when port is 1023, execute "if (port > endport)"
thread B: execute "sin->sin_port = htons (port++);", port is 1024
thread A: execute "res = __bind (sd, sin, sizeof (struct sockaddr_in));"

So, static should be replaced with static __thread.

-- 
Best Regards,
Peng

>>    struct sockaddr_in myaddr;
>>    int i;
>>
>> @@ -66,7 +68,7 @@ bindresvport (int sd, struct sockaddr_in *sin)
>>
>>    if (port == 0)
>>      {
>> -      port = (__getpid () % NPORTS) + STARTPORT;
>> +      port = (get_port () % NPORTS) + STARTPORT;
>>      }
>>
>>    /* Initialize to make gcc happy.  */
>> diff --git a/sunrpc/bindrsvprt.h b/sunrpc/bindrsvprt.h
>> new file mode 100644
>> index 0000000..221f811
>> --- /dev/null
>> +++ b/sunrpc/bindrsvprt.h
>> @@ -0,0 +1,37 @@
>> +/* Copyright (C) 2012 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifdef __linux__
>> +#ifdef INTERNAL_SYSCALL
>> +#define get_port()                            \
> 
> Call this __get_uid(), since that's what you're doing.
> 
> You are getting a unique id for the process or thread
> and using that in the hash function to find a good port
> to start the bind attempt.
> 
>> + ({                                           \
>> +  INTERNAL_SYSCALL_DECL (err);                \
>> +  short __port;                               \
>> +  __port = INTERNAL_SYSCALL (gettid, err, 0); \
>> +  __port;                                     \
>> + })
>> +#else
>> +#define get_port()                            \
>> + ({                                           \
>> +  short __port;                               \
>> +  __port = INLINE_SYSCALL (gettid, 0);        \
>> +  __port;                                     \
>> + })
>> +#endif /* INTERNAL_SYSCALL */
>> +#else
>> +#define get_port() __getpid()
>> +#endif /* __linux__ */
>> --
>> 1.7.11.4
>>
> 
> Cheers,
> Carlos.
> 

-- 
Best Regards,
Peng