This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] Make bindresvport() function to multithread-safe
- From: "Carlos O'Donell" <carlos at systemhalted dot org>
- To: Peng Haitao <penght at cn dot fujitsu dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>, Roland McGrath <roland at hack dot frob dot com>, Andreas Jaeger <aj at suse dot com>
- Date: Fri, 28 Sep 2012 10:27:56 -0400
- Subject: Re: [PATCH v4] Make bindresvport() function to multithread-safe
- References: <1348823725-18793-1-git-send-email-penght@cn.fujitsu.com>
On Fri, Sep 28, 2012 at 5:15 AM, Peng Haitao <penght@cn.fujitsu.com> wrote:
> bindresvport() uses a static variable port which is not protected.
> It is not safe when in multithread circumstance.
>
> bindresvport() select a port number from the range 512 to 1023, when in
> multithread circumstance, the port may be 1024. So the static variable
> will be protected.
Roland, Andreas,
Do you have any comments on this?
Looks like performance goes *up* when we switch to a __thread variable
and use gettid, but I've asked Peng to justify this.
http://sourceware.org/ml/libc-alpha/2012-09/msg00744.html
> Signed-off-by: Peng Haitao <penght@cn.fujitsu.com>
> ---
> ChangeLog | 6 ++++++
> sunrpc/bindrsvprt.c | 8 +++++---
> sunrpc/bindrsvprt.h | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 3 deletions(-)
> create mode 100644 sunrpc/bindrsvprt.h
>
> diff --git a/ChangeLog b/ChangeLog
> index 3004989..8db0157 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2012-09-27 Peng Haitao <penght@cn.fujitsu.com>
> +
> + [BZ #13763]
> + * sunrpc/bindrsvprt.c: Add __thread when define static's port,
> + getpid() is changed to gettid() in Linux.
> +
> 2012-09-27 David S. Miller <davem@davemloft.net>
>
> [BZ #14376]
> diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
> index d493c9f..ab835a1 100644
> --- a/sunrpc/bindrsvprt.c
> +++ b/sunrpc/bindrsvprt.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2010, Oracle America, Inc.
> + * Copyright (c) 2010-2012, Oracle America, Inc.
Is this correct? I don't know that we have a ruling on changing Copyright
headers for copyright that the FSF doesn't own.
Have we established a rule here?
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -36,13 +36,15 @@
> #include <sys/socket.h>
> #include <netinet/in.h>
>
> +#include "bindrsvprt.h"
Can you make this #include <bindrsvprt.h> and allow the normal sysdep
scanning to pick it up?
By using <> you allow other machines to override this if required.
> +
> /*
> * 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?
> 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.