* Re: discrepancy in ip(7) wrt. IP DF flag for UDP sockets
From: Benjamin Poirier @ 2011-09-20 13:29 UTC (permalink / raw)
To: Michael Kerrisk
Cc: Neil Horman, linux-man-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAKgNAkgWRdkxgTXNnMi4PVGGsOd-8EHF1siBrk-Bj=rnYenVmQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11-09-20 08:14, Michael Kerrisk wrote:
> Hello Benjamin, Neil,
>
> On Mon, Sep 19, 2011 at 3:03 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> > On Mon, Sep 19, 2011 at 08:19:40AM -0400, Benjamin Poirier wrote:
> >> Hi,
> >>
> >> I noticed what appears to be a discrepancy between the ip(7) man page
> >> and the kernel code with regards to the IP DF flag for UDP sockets.
> >>
> >> The man page says that "The don't-fragment flag is set on all outgoing
> >> datagrams" and that the ip_no_pmtu_disc sysctl affects only SOCK_STREAM
> >> sockets. This is quickly disproved by doing:
> >> echo 1 > /proc/sys/net/ipv4/ip_no_pmtu_disc
> >> firing up netcat and looking at a few outgoing udp packets in wireshark
> >> (they don't have the DF flag set).
>
> Could you describe the required change in terms of how the man page
> text should look--i.e., rewrite the passage as you think it should
> look?
How about changing it to:
IP_MTU_DISCOVER (since Linux 2.2)
Set or receive the Path MTU Discovery setting for a socket. When
enabled, the don't-fragment flag is set on all outgoing packets.
Linux will perform Path MTU Discovery as defined in RFC 1191 on
SOCK_STREAM sockets. For non-SOCK_STREAM sockets, it is the
user's responsibility to packetize the data in MTU sized chunks
and to do the retransmits if necessary. The kernel will reject
(with EMSGSIZE) datagrams that are bigger than the known path
MTU. The system-wide default is controlled by the
/proc/sys/net/ipv4/ip_no_pmtu_disc file.
Path MTU discovery flags Meaning
[...]
There are some differences between _DO and _WANT that are glossed over
in this description, but I suppose there's only so much detail you can
put in...
Thanks,
-Ben
>
> Thanks,
>
> Michael
>
>
> >> 1) in the words of `man 7 ip`:
> >> IP_MTU_DISCOVER (since Linux 2.2)
> >> Set or receive the Path MTU Discovery setting for a socket.
> >> When enabled, Linux will perform Path MTU Discovery as defined
> >> in RFC 1191 on this socket. The don't-fragment flag is set on
> >> all outgoing datagrams. The system-wide default is controlled
> >> by the /proc/sys/net/ipv4/ip_no_pmtu_disc file for SOCK_STREAM
> >> sockets, and disabled on all others.
> >>
> >> This is the text present in the latest version of the online manpages,
> >> http://webcache.googleusercontent.com/search?q=cache:http://www.kernel.org/doc/man-pages/reporting_bugs.html&ie=UTF-8
> >>
> >> 2) in net/ipv4/af_inet.c:inet_create():
> >> if (ipv4_config.no_pmtu_disc)
> >> inet->pmtudisc = IP_PMTUDISC_DONT;
> >> else
> >> inet->pmtudisc = IP_PMTUDISC_WANT;
> >>
> >> and pmtudisc is left alone from there on for UDP sockets.
> >>
> >> What should be adjusted, the man page or the code?
> >>
> > The man page is wrong I think
> >
> > By my read, the code:
> > 1) Affects UDP and TCP the same way (which makes sense to me)
> >
> > 2) Is doing exactly what you asked it to, since you set no_pmtu_disc, which
> > means the stack should be free to fragment a frame as it sees fit according to
> > the MTU metric of the route its traversing, hence the cleared DF bit in the
> > fraem.
> >
> > RFC 1191 can apply equally well to udp, as tcp, and is evident in that you can
> > set the per-socket option IP_MTU_DISCOVER to any of the 4 acceptible values
> > offered (DONT/WANT/DO/PROBE), so theres no reason the sysctl governing the
> > default value at creation shouldn't apply as well.
> > Neil
> >
> >
>
>
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 13:34 UTC (permalink / raw)
To: Myklebust, Trond
Cc: Schumaker, Bryan, linux-nfs@vger.kernel.org, Pavel Emelianov,
neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <2E1EB2CF9ED1CB4AA966F0EB76EAB4430B47FD10@SACMVEXC2-PRD.hq.netapp.com>
20.09.2011 17:15, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Schumaker, Bryan
>> Sent: Tuesday, September 20, 2011 9:05 AM
>> To: Stanislav Kinsbursky
>> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
>>> This helpers will be used for dynamical creation and destruction of
>>> rpcbind clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services. If
>>> rpcbind clients has been created already, then we just increase rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>> net/sunrpc/rpcb_clnt.c | 50
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 files changed, 50 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
>>> e45d2fb..8724780 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>>> static struct rpc_clnt * rpcb_local_clnt;
>>> static struct rpc_clnt * rpcb_local_clnt4;
>>>
>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int rpcb_users;
>>> +
>>> struct rpcbind_args {
>>> struct rpc_xprt * r_xprt;
>>>
>>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
>>> kfree(map);
>>> }
>>>
>>> +static int rpcb_get_local(void)
>>> +{
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (rpcb_users)
>>> + rpcb_users++;
>>> + spin_unlock(&rpcb_clnt_lock);
>>> +
>>> + return rpcb_users;
>> ^^^^^^^^^^^^^^^^^^
>> Is it safe to use this variable outside of the rpcb_clnt_lock?
>>
> Nope. If rpcb_users was zero in the protected section above, nothing guarantees that it will still be zero here, and so the caller may get the (wrong) impression that the counter was incremented.
>
Yep, you right. Will fix this races.
>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> + int shutdown;
>>> +
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (--rpcb_users == 0) {
>>> + rpcb_local_clnt = NULL;
>>> + rpcb_local_clnt4 = NULL;
>>> + }
>>> + shutdown = !rpcb_users;
>>> + spin_unlock(&rpcb_clnt_lock);
>>> +
>>> + if (shutdown) {
>>> + /*
>>> + * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>>> + */
>>> + if (clnt4)
>>> + rpc_shutdown_client(clnt4);
>>> + if (clnt)
>>> + rpc_shutdown_client(clnt);
>>> + }
>>> + return;
>>> +}
>>> +
>>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
>>> +*clnt4) {
>>> + /* Protected by rpcb_create_local_mutex */
>
> Doesn't it need to be protected by rpcb_clnt_lock too?
>
Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one will
change rpcb_users since it's zero. If it's non zero - we willn't get to
rpcb_set_local().
>>> + rpcb_local_clnt = clnt;
>>> + rpcb_local_clnt4 = clnt4;
>>> + rpcb_users++;
> ^^^^^^^^^^^^^^^^^^^
>
>>> + dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
>>> + "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>>> + rpcb_local_clnt4);
>>> +}
>>> +
>>> /*
>>> * Returns zero on success, otherwise a negative errno value
>>> * is returned.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
>>> in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>>> info at http://vger.kernel.org/majordomo-info.html
>
> \x04�{.n�+�������+%��lzwm��b�맲��r��zX��\x19߲)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* Re: discrepancy in ip(7) wrt. IP DF flag for UDP sockets
From: Neil Horman @ 2011-09-20 13:38 UTC (permalink / raw)
To: Benjamin Poirier
Cc: Michael Kerrisk, linux-man-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110920132954.GA23041-k/PPzeaMb74v2OKnPYDugg@public.gmane.org>
On Tue, Sep 20, 2011 at 09:29:54AM -0400, Benjamin Poirier wrote:
> On 11-09-20 08:14, Michael Kerrisk wrote:
> > Hello Benjamin, Neil,
> >
> > On Mon, Sep 19, 2011 at 3:03 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> > > On Mon, Sep 19, 2011 at 08:19:40AM -0400, Benjamin Poirier wrote:
> > >> Hi,
> > >>
> > >> I noticed what appears to be a discrepancy between the ip(7) man page
> > >> and the kernel code with regards to the IP DF flag for UDP sockets.
> > >>
> > >> The man page says that "The don't-fragment flag is set on all outgoing
> > >> datagrams" and that the ip_no_pmtu_disc sysctl affects only SOCK_STREAM
> > >> sockets. This is quickly disproved by doing:
> > >> echo 1 > /proc/sys/net/ipv4/ip_no_pmtu_disc
> > >> firing up netcat and looking at a few outgoing udp packets in wireshark
> > >> (they don't have the DF flag set).
> >
> > Could you describe the required change in terms of how the man page
> > text should look--i.e., rewrite the passage as you think it should
> > look?
>
> How about changing it to:
> IP_MTU_DISCOVER (since Linux 2.2)
> Set or receive the Path MTU Discovery setting for a socket. When
> enabled, the don't-fragment flag is set on all outgoing packets.
> Linux will perform Path MTU Discovery as defined in RFC 1191 on
> SOCK_STREAM sockets. For non-SOCK_STREAM sockets, it is the
> user's responsibility to packetize the data in MTU sized chunks
> and to do the retransmits if necessary. The kernel will reject
> (with EMSGSIZE) datagrams that are bigger than the known path
> MTU. The system-wide default is controlled by the
> /proc/sys/net/ipv4/ip_no_pmtu_disc file.
>
> Path MTU discovery flags Meaning
> [...]
>
> There are some differences between _DO and _WANT that are glossed over
> in this description, but I suppose there's only so much detail you can
> put in...
>
> Thanks,
> -Ben
>
Yeah, I think thats close, but its only the users responsibility to package
datagrams in mtu sized chunks if they force the dont fragment bit on. If they
go wtih the default, the stack will fragment a datagram is it sees fit according
to the mtu of the path it traverses
Neil
> >
> > Thanks,
> >
> > Michael
> >
> >
> > >> 1) in the words of `man 7 ip`:
> > >> IP_MTU_DISCOVER (since Linux 2.2)
> > >> Set or receive the Path MTU Discovery setting for a socket.
> > >> When enabled, Linux will perform Path MTU Discovery as defined
> > >> in RFC 1191 on this socket. The don't-fragment flag is set on
> > >> all outgoing datagrams. The system-wide default is controlled
> > >> by the /proc/sys/net/ipv4/ip_no_pmtu_disc file for SOCK_STREAM
> > >> sockets, and disabled on all others.
> > >>
> > >> This is the text present in the latest version of the online manpages,
> > >> http://webcache.googleusercontent.com/search?q=cache:http://www.kernel.org/doc/man-pages/reporting_bugs.html&ie=UTF-8
> > >>
> > >> 2) in net/ipv4/af_inet.c:inet_create():
> > >> if (ipv4_config.no_pmtu_disc)
> > >> inet->pmtudisc = IP_PMTUDISC_DONT;
> > >> else
> > >> inet->pmtudisc = IP_PMTUDISC_WANT;
> > >>
> > >> and pmtudisc is left alone from there on for UDP sockets.
> > >>
> > >> What should be adjusted, the man page or the code?
> > >>
> > > The man page is wrong I think
> > >
> > > By my read, the code:
> > > 1) Affects UDP and TCP the same way (which makes sense to me)
> > >
> > > 2) Is doing exactly what you asked it to, since you set no_pmtu_disc, which
> > > means the stack should be free to fragment a frame as it sees fit according to
> > > the MTU metric of the route its traversing, hence the cleared DF bit in the
> > > fraem.
> > >
> > > RFC 1191 can apply equally well to udp, as tcp, and is evident in that you can
> > > set the per-socket option IP_MTU_DISCOVER to any of the 4 acceptible values
> > > offered (DONT/WANT/DO/PROBE), so theres no reason the sysctl governing the
> > > default value at creation shouldn't apply as well.
> > > Neil
> > >
> > >
> >
> >
> >
> > --
> > Michael Kerrisk
> > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> > Author of "The Linux Programming Interface"; http://man7.org/tlpi/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 13:49 UTC (permalink / raw)
To: Trond.Myklebust@netapp.com
Cc: linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bfields@fieldses.org, davem@davemloft.net
In-Reply-To: <20110920101341.9861.51453.stgit@localhost6.localdomain6>
v5: fixed races with rpcb_users in rpcb_get_local()
This helpers will be used for dynamical creation and destruction of rpcbind
clients.
Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
clients has been created already, then we just increase rpcb_users.
Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 53 insertions(+), 0 deletions(-)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..5f4a406 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
static struct rpc_clnt * rpcb_local_clnt;
static struct rpc_clnt * rpcb_local_clnt4;
+DEFINE_SPINLOCK(rpcb_clnt_lock);
+unsigned int rpcb_users;
+
struct rpcbind_args {
struct rpc_xprt * r_xprt;
@@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
kfree(map);
}
+static int rpcb_get_local(void)
+{
+ int cnt;
+
+ spin_lock(&rpcb_clnt_lock);
+ if (rpcb_users)
+ rpcb_users++;
+ cnt = rpcb_users;
+ spin_unlock(&rpcb_clnt_lock);
+
+ return cnt;
+}
+
+void rpcb_put_local(void)
+{
+ struct rpc_clnt *clnt = rpcb_local_clnt;
+ struct rpc_clnt *clnt4 = rpcb_local_clnt4;
+ int shutdown;
+
+ spin_lock(&rpcb_clnt_lock);
+ if (--rpcb_users == 0) {
+ rpcb_local_clnt = NULL;
+ rpcb_local_clnt4 = NULL;
+ }
+ shutdown = !rpcb_users;
+ spin_unlock(&rpcb_clnt_lock);
+
+ if (shutdown) {
+ /*
+ * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
+ */
+ if (clnt4)
+ rpc_shutdown_client(clnt4);
+ if (clnt)
+ rpc_shutdown_client(clnt);
+ }
+ return;
+}
+
+static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
+{
+ /* Protected by rpcb_create_local_mutex */
+ rpcb_local_clnt = clnt;
+ rpcb_local_clnt4 = clnt4;
+ rpcb_users++;
+ dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
+ "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
+ rpcb_local_clnt4);
+}
+
/*
* Returns zero on success, otherwise a negative errno value
* is returned.
^ permalink raw reply related
* Re: discrepancy in ip(7) wrt. IP DF flag for UDP sockets
From: Benjamin Poirier @ 2011-09-20 14:12 UTC (permalink / raw)
To: Neil Horman
Cc: Michael Kerrisk, linux-man-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110920133837.GA16323-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org>
On 11-09-20 09:38, Neil Horman wrote:
> On Tue, Sep 20, 2011 at 09:29:54AM -0400, Benjamin Poirier wrote:
> > On 11-09-20 08:14, Michael Kerrisk wrote:
> > > Hello Benjamin, Neil,
> > >
[snip]
> > >
> > > Could you describe the required change in terms of how the man page
> > > text should look--i.e., rewrite the passage as you think it should
> > > look?
> >
> > How about changing it to:
> > IP_MTU_DISCOVER (since Linux 2.2)
> > Set or receive the Path MTU Discovery setting for a socket. When
> > enabled, the don't-fragment flag is set on all outgoing packets.
> > Linux will perform Path MTU Discovery as defined in RFC 1191 on
> > SOCK_STREAM sockets. For non-SOCK_STREAM sockets, it is the
> > user's responsibility to packetize the data in MTU sized chunks
> > and to do the retransmits if necessary. The kernel will reject
> > (with EMSGSIZE) datagrams that are bigger than the known path
> > MTU. The system-wide default is controlled by the
> > /proc/sys/net/ipv4/ip_no_pmtu_disc file.
> >
> > Path MTU discovery flags Meaning
> > [...]
> >
> > There are some differences between _DO and _WANT that are glossed over
> > in this description, but I suppose there's only so much detail you can
> > put in...
> >
> > Thanks,
> > -Ben
> >
> Yeah, I think thats close, but its only the users responsibility to package
> datagrams in mtu sized chunks if they force the dont fragment bit on. If they
> go wtih the default, the stack will fragment a datagram is it sees fit according
> to the mtu of the path it traverses
Exactly. To get into this level of detail, I think we have to mention
the option value, not just enabled/disabled. Let's try like this:
IP_MTU_DISCOVER (since Linux 2.2)
Set or receive the Path MTU Discovery setting for a socket. When
enabled, Linux will perform Path MTU Discovery as defined in RFC
1191 on SOCK_STREAM sockets. For non-SOCK_STREAM sockets,
IP_PMTUDISC_DO forces the don't-fragment flag to be set on all
outgoing packets. It is the user's responsibility to packetize
the data in MTU sized chunks and to do the retransmits if
necessary. The kernel will reject (with EMSGSIZE) datagrams
that are bigger than the known path MTU. IP_PMTUDISC_WANT will
fragment a datagram if needed according to the path MTU or will
set the don't-fragment flag otherwise.
The system-wide default can be toggled between IP_PMTUDISC_WANT
and IP_PMTUDISC_DONT by writting to the
/proc/sys/net/ipv4/ip_no_pmtu_disc file.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Myklebust, Trond @ 2011-09-20 14:14 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Schumaker, Bryan, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
Pavel Emelianov, neilb-l3A5Bk7waGM, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bfields-uC3wQj2KruNg9hUCZPvPmw, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <4E789679.1060601-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 9:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
>
> 20.09.2011 17:15, Myklebust, Trond пишет:
> >> -----Original Message-----
> >> From: Schumaker, Bryan
> >> Sent: Tuesday, September 20, 2011 9:05 AM
> >> To: Stanislav Kinsbursky
> >> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com;
> >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> bfields@fieldses.org; davem@davemloft.net
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> >>> This helpers will be used for dynamical creation and destruction of
> >>> rpcbind clients.
> >>> Variable rpcb_users is actually a counter of lauched RPC services.
> >>> If rpcbind clients has been created already, then we just increase
> rpcb_users.
> >>>
> >>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
> >>>
> >>> ---
> >>> net/sunrpc/rpcb_clnt.c | 50
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 files changed, 50 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> >>> e45d2fb..8724780 100644
> >>> --- a/net/sunrpc/rpcb_clnt.c
> >>> +++ b/net/sunrpc/rpcb_clnt.c
> >>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
> >>> static struct rpc_clnt * rpcb_local_clnt;
> >>> static struct rpc_clnt * rpcb_local_clnt4;
> >>>
> >>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >>> +unsigned int rpcb_users;
> >>> +
> >>> struct rpcbind_args {
> >>> struct rpc_xprt * r_xprt;
> >>>
> >>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
> >>> kfree(map);
> >>> }
> >>>
> >>> +static int rpcb_get_local(void)
> >>> +{
> >>> + spin_lock(&rpcb_clnt_lock);
> >>> + if (rpcb_users)
> >>> + rpcb_users++;
> >>> + spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> + return rpcb_users;
> >> ^^^^^^^^^^^^^^^^^^
> >> Is it safe to use this variable outside of the rpcb_clnt_lock?
> >>
> > Nope. If rpcb_users was zero in the protected section above, nothing
> guarantees that it will still be zero here, and so the caller may get the (wrong)
> impression that the counter was incremented.
> >
>
> Yep, you right. Will fix this races.
>
> >>> +}
> >>> +
> >>> +void rpcb_put_local(void)
> >>> +{
> >>> + struct rpc_clnt *clnt = rpcb_local_clnt;
> >>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >>> + int shutdown;
> >>> +
> >>> + spin_lock(&rpcb_clnt_lock);
> >>> + if (--rpcb_users == 0) {
> >>> + rpcb_local_clnt = NULL;
> >>> + rpcb_local_clnt4 = NULL;
> >>> + }
> >>> + shutdown = !rpcb_users;
> >>> + spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> + if (shutdown) {
> >>> + /*
> >>> + * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> >>> + */
> >>> + if (clnt4)
> >>> + rpc_shutdown_client(clnt4);
> >>> + if (clnt)
> >>> + rpc_shutdown_client(clnt);
> >>> + }
> >>> + return;
> >>> +}
> >>> +
> >>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
> >>> +*clnt4) {
> >>> + /* Protected by rpcb_create_local_mutex */
> >
> > Doesn't it need to be protected by rpcb_clnt_lock too?
> >
>
> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
> will change rpcb_users since it's zero. If it's non zero - we willn't get to
> rpcb_set_local().
OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?
In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
> >>> + rpcb_local_clnt = clnt;
> >>> + rpcb_local_clnt4 = clnt4;
> >>> + rpcb_users++;
> > ^^^^^^^^^^^^^^^^^^^
> >
> >>> + dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
> >>> + "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> >>> + rpcb_local_clnt4);
> >>> +}
> >>> +
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Jeff Layton @ 2011-09-20 14:24 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <4E7899E7.9090809@parallels.com>
On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:
> v5: fixed races with rpcb_users in rpcb_get_local()
>
> This helpers will be used for dynamical creation and destruction of rpcbind
> clients.
> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> clients has been created already, then we just increase rpcb_users.
>
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>
> ---
> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index e45d2fb..5f4a406 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
> static struct rpc_clnt * rpcb_local_clnt;
> static struct rpc_clnt * rpcb_local_clnt4;
> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> +unsigned int rpcb_users;
> +
> struct rpcbind_args {
> struct rpc_xprt * r_xprt;
> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
> kfree(map);
> }
> +static int rpcb_get_local(void)
> +{
> + int cnt;
> +
> + spin_lock(&rpcb_clnt_lock);
> + if (rpcb_users)
> + rpcb_users++;
> + cnt = rpcb_users;
> + spin_unlock(&rpcb_clnt_lock);
> +
> + return cnt;
> +}
> +
> +void rpcb_put_local(void)
> +{
> + struct rpc_clnt *clnt = rpcb_local_clnt;
> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> + int shutdown;
> +
> + spin_lock(&rpcb_clnt_lock);
> + if (--rpcb_users == 0) {
> + rpcb_local_clnt = NULL;
> + rpcb_local_clnt4 = NULL;
> + }
In the function below, you mention that the above pointers are
protected by rpcb_create_local_mutex, but it looks like they get reset
here without that being held?
Might it be simpler to just protect rpcb_users with the
rpcb_create_local_mutex and ensure that it's held whenever you call one
of these routines? None of these are codepaths are particularly hot.
> + shutdown = !rpcb_users;
> + spin_unlock(&rpcb_clnt_lock);
> +
> + if (shutdown) {
> + /*
> + * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> + */
> + if (clnt4)
> + rpc_shutdown_client(clnt4);
> + if (clnt)
> + rpc_shutdown_client(clnt);
> + }
> + return;
> +}
> +
> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
> +{
> + /* Protected by rpcb_create_local_mutex */
> + rpcb_local_clnt = clnt;
> + rpcb_local_clnt4 = clnt4;
> + rpcb_users++;
> + dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
> + "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> + rpcb_local_clnt4);
> +}
> +
> /*
> * Returns zero on success, otherwise a negative errno value
> * is returned.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply
* Re: pull request: wireless-next 2011-09-19
From: John W. Linville @ 2011-09-20 14:18 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev
In-Reply-To: <20110919203639.GF2608@tuxdriver.com>
On Mon, Sep 19, 2011 at 04:36:39PM -0400, John W. Linville wrote:
> David,
>
> Here is another big batch of updates intended for 3.2 -- still
> clearing-out the backlog...
>
> This round includes lots of updates to ath9k, b43, iwlagn, and rt2x00.
> Some cleanups go to mac80211, along with a number of mesh-mode fixes
> from Javier.
>
> Please let me know if there are problems!
>
> Thanks,
>
> John
>
> ---
>
> The following changes since commit 765cf9976e937f1cfe9159bf4534967c8bf8eb6d:
>
> tcp: md5: remove one indirection level in tcp_md5sig_pool (2011-09-17 01:15:46 -0400)
>
> are available in the git repository at:
> ssh://infradead/~/public_git/wireless-next.git for-davem
This is also available at this URL -- sorry about the ssh URL (with
an alias at that)!
git://git.infradead.org/users/linville/wireless-next.git for-davem
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: discrepancy in ip(7) wrt. IP DF flag for UDP sockets
From: Neil Horman @ 2011-09-20 14:31 UTC (permalink / raw)
To: Benjamin Poirier
Cc: Michael Kerrisk, linux-man-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110920141234.GA23164-k/PPzeaMb74v2OKnPYDugg@public.gmane.org>
On Tue, Sep 20, 2011 at 10:12:34AM -0400, Benjamin Poirier wrote:
> On 11-09-20 09:38, Neil Horman wrote:
> > On Tue, Sep 20, 2011 at 09:29:54AM -0400, Benjamin Poirier wrote:
> > > On 11-09-20 08:14, Michael Kerrisk wrote:
> > > > Hello Benjamin, Neil,
> > > >
> [snip]
> > > >
> > > > Could you describe the required change in terms of how the man page
> > > > text should look--i.e., rewrite the passage as you think it should
> > > > look?
> > >
> > > How about changing it to:
> > > IP_MTU_DISCOVER (since Linux 2.2)
> > > Set or receive the Path MTU Discovery setting for a socket. When
> > > enabled, the don't-fragment flag is set on all outgoing packets.
> > > Linux will perform Path MTU Discovery as defined in RFC 1191 on
> > > SOCK_STREAM sockets. For non-SOCK_STREAM sockets, it is the
> > > user's responsibility to packetize the data in MTU sized chunks
> > > and to do the retransmits if necessary. The kernel will reject
> > > (with EMSGSIZE) datagrams that are bigger than the known path
> > > MTU. The system-wide default is controlled by the
> > > /proc/sys/net/ipv4/ip_no_pmtu_disc file.
> > >
> > > Path MTU discovery flags Meaning
> > > [...]
> > >
> > > There are some differences between _DO and _WANT that are glossed over
> > > in this description, but I suppose there's only so much detail you can
> > > put in...
> > >
> > > Thanks,
> > > -Ben
> > >
> > Yeah, I think thats close, but its only the users responsibility to package
> > datagrams in mtu sized chunks if they force the dont fragment bit on. If they
> > go wtih the default, the stack will fragment a datagram is it sees fit according
> > to the mtu of the path it traverses
>
> Exactly. To get into this level of detail, I think we have to mention
> the option value, not just enabled/disabled. Let's try like this:
>
> IP_MTU_DISCOVER (since Linux 2.2)
> Set or receive the Path MTU Discovery setting for a socket. When
> enabled, Linux will perform Path MTU Discovery as defined in RFC
> 1191 on SOCK_STREAM sockets. For non-SOCK_STREAM sockets,
> IP_PMTUDISC_DO forces the don't-fragment flag to be set on all
> outgoing packets. It is the user's responsibility to packetize
> the data in MTU sized chunks and to do the retransmits if
> necessary. The kernel will reject (with EMSGSIZE) datagrams
> that are bigger than the known path MTU. IP_PMTUDISC_WANT will
> fragment a datagram if needed according to the path MTU or will
> set the don't-fragment flag otherwise.
>
> The system-wide default can be toggled between IP_PMTUDISC_WANT
> and IP_PMTUDISC_DONT by writting to the
> /proc/sys/net/ipv4/ip_no_pmtu_disc file.
>
Yes, that sounds good to me. Thanks for doing this!
Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 14:35 UTC (permalink / raw)
To: Myklebust, Trond
Cc: Schumaker, Bryan, linux-nfs@vger.kernel.org, Pavel Emelianov,
neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <2E1EB2CF9ED1CB4AA966F0EB76EAB4430B47FD22@SACMVEXC2-PRD.hq.netapp.com>
20.09.2011 18:14, Myklebust, Trond пишет:
>>>
>>> Doesn't it need to be protected by rpcb_clnt_lock too?
>>>
>>
>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
>> will change rpcb_users since it's zero. If it's non zero - we willn't get to
>> rpcb_set_local().
>
> OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?
>
Yes, you right.
> In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>
We check rpcb_users under spinlock. Gain spinlock forces memory barrier, doesn't it?
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Myklebust, Trond @ 2011-09-20 14:38 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Schumaker, Bryan, linux-nfs, Pavel Emelianov, neilb, netdev,
linux-kernel, bfields, davem
In-Reply-To: <4E78A4AF.1020303@parallels.com>
> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 10:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
>
> 20.09.2011 18:14, Myklebust, Trond пишет:
>
> >>>
> >>> Doesn't it need to be protected by rpcb_clnt_lock too?
> >>>
> >>
> >> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
> >> one will change rpcb_users since it's zero. If it's non zero - we
> >> willn't get to rpcb_set_local().
> >
> > OK, so you are saying that the rpcb_users++ below could be replaced by
> rpcb_users=1?
> >
>
> Yes, you right.
>
> > In that case, don't you need a smp_wmb() between the setting of
> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
> >
>
> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
> doesn't it?
Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.
Cheers
Trond
^ permalink raw reply
* RE: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Myklebust, Trond @ 2011-09-20 14:41 UTC (permalink / raw)
To: Jeff Layton, Stanislav Kinsbursky
Cc: linux-nfs, Pavel Emelianov, neilb, netdev, linux-kernel, bfields,
davem
In-Reply-To: <20110920102431.58ca1d96@tlielax.poochiereds.net>
> -----Original Message-----
> From: Jeff Layton [mailto:jlayton@redhat.com]
> Sent: Tuesday, September 20, 2011 10:25 AM
> To: Stanislav Kinsbursky
> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
>
> On Tue, 20 Sep 2011 17:49:27 +0400
> Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:
>
> > v5: fixed races with rpcb_users in rpcb_get_local()
> >
> > This helpers will be used for dynamical creation and destruction of
> > rpcbind clients.
> > Variable rpcb_users is actually a counter of lauched RPC services.
If
> > rpcbind clients has been created already, then we just increase
rpcb_users.
> >
> > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> >
> > ---
> > net/sunrpc/rpcb_clnt.c | 53
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 53 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> > e45d2fb..5f4a406 100644
> > --- a/net/sunrpc/rpcb_clnt.c
> > +++ b/net/sunrpc/rpcb_clnt.c
> > @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
> > static struct rpc_clnt * rpcb_local_clnt;
> > static struct rpc_clnt * rpcb_local_clnt4;
> > +DEFINE_SPINLOCK(rpcb_clnt_lock);
> > +unsigned int rpcb_users;
> > +
> > struct rpcbind_args {
> > struct rpc_xprt * r_xprt;
> > @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
> > kfree(map);
> > }
> > +static int rpcb_get_local(void)
> > +{
> > + int cnt;
> > +
> > + spin_lock(&rpcb_clnt_lock);
> > + if (rpcb_users)
> > + rpcb_users++;
> > + cnt = rpcb_users;
> > + spin_unlock(&rpcb_clnt_lock);
> > +
> > + return cnt;
> > +}
> > +
> > +void rpcb_put_local(void)
> > +{
> > + struct rpc_clnt *clnt = rpcb_local_clnt;
> > + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> > + int shutdown;
> > +
> > + spin_lock(&rpcb_clnt_lock);
> > + if (--rpcb_users == 0) {
> > + rpcb_local_clnt = NULL;
> > + rpcb_local_clnt4 = NULL;
> > + }
>
> In the function below, you mention that the above pointers are
protected by
> rpcb_create_local_mutex, but it looks like they get reset here without
that
> being held?
>
> Might it be simpler to just protect rpcb_users with the
> rpcb_create_local_mutex and ensure that it's held whenever you call
one of
> these routines? None of these are codepaths are particularly hot.
Alternatively, if you do
if (rpcb_users == 1) {
rpcb_local_clnt = NULL;
rpcb_local_clnt4 = NULL;
smp_wmb();
rpcb_users = 0;
} else
rpcb_users--;
then the spinlock protection in rpbc_get_local() is still good enough to
guarantee correctness.
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 14:43 UTC (permalink / raw)
To: Jeff Layton
Cc: Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org,
linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20110920102431.58ca1d96-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
20.09.2011 18:24, Jeff Layton пишет:
> On Tue, 20 Sep 2011 17:49:27 +0400
> Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:
>
>> v5: fixed races with rpcb_users in rpcb_get_local()
>>
>> This helpers will be used for dynamical creation and destruction of rpcbind
>> clients.
>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>> clients has been created already, then we just increase rpcb_users.
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>
>> ---
>> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 53 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>> index e45d2fb..5f4a406 100644
>> --- a/net/sunrpc/rpcb_clnt.c
>> +++ b/net/sunrpc/rpcb_clnt.c
>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>> static struct rpc_clnt * rpcb_local_clnt;
>> static struct rpc_clnt * rpcb_local_clnt4;
>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>> +unsigned int rpcb_users;
>> +
>> struct rpcbind_args {
>> struct rpc_xprt * r_xprt;
>> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>> kfree(map);
>> }
>> +static int rpcb_get_local(void)
>> +{
>> + int cnt;
>> +
>> + spin_lock(&rpcb_clnt_lock);
>> + if (rpcb_users)
>> + rpcb_users++;
>> + cnt = rpcb_users;
>> + spin_unlock(&rpcb_clnt_lock);
>> +
>> + return cnt;
>> +}
>> +
>> +void rpcb_put_local(void)
>> +{
>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>> + int shutdown;
>> +
>> + spin_lock(&rpcb_clnt_lock);
>> + if (--rpcb_users == 0) {
>> + rpcb_local_clnt = NULL;
>> + rpcb_local_clnt4 = NULL;
>> + }
>
> In the function below, you mention that the above pointers are
> protected by rpcb_create_local_mutex, but it looks like they get reset
> here without that being held?
>
Assigning of them is protected by rpcb_create_local_mutex.
Dereferencing of them is protected by rpcb_clnt_lock.
> Might it be simpler to just protect rpcb_users with the
> rpcb_create_local_mutex and ensure that it's held whenever you call one
> of these routines? None of these are codepaths are particularly hot.
>
I just inherited this lock-mutex logic.
Actually, you right. This codepaths are used rarely.
But are use sure, that we need to remove this "speed-up" logic if we take into
account that it was here already?
>> + shutdown = !rpcb_users;
>> + spin_unlock(&rpcb_clnt_lock);
>> +
>> + if (shutdown) {
>> + /*
>> + * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>> + */
>> + if (clnt4)
>> + rpc_shutdown_client(clnt4);
>> + if (clnt)
>> + rpc_shutdown_client(clnt);
>> + }
>> + return;
>> +}
>> +
>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
>> +{
>> + /* Protected by rpcb_create_local_mutex */
>> + rpcb_local_clnt = clnt;
>> + rpcb_local_clnt4 = clnt4;
>> + rpcb_users++;
>> + dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
>> + "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>> + rpcb_local_clnt4);
>> +}
>> +
>> /*
>> * Returns zero on success, otherwise a negative errno value
>> * is returned.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Best regards,
Stanislav Kinsbursky
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Bryan Schumaker @ 2011-09-20 14:58 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Jeff Layton, Trond.Myklebust@netapp.com,
linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bfields@fieldses.org, davem@davemloft.net
In-Reply-To: <4E78A6A1.1000800@parallels.com>
On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
> 20.09.2011 18:24, Jeff Layton пишет:
>> On Tue, 20 Sep 2011 17:49:27 +0400
>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:
>>
>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>
>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>> clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>> clients has been created already, then we just increase rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>> index e45d2fb..5f4a406 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>>> static struct rpc_clnt * rpcb_local_clnt;
>>> static struct rpc_clnt * rpcb_local_clnt4;
>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int rpcb_users;
>>> +
>>> struct rpcbind_args {
>>> struct rpc_xprt * r_xprt;
>>> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>> kfree(map);
>>> }
>>> +static int rpcb_get_local(void)
>>> +{
>>> + int cnt;
>>> +
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (rpcb_users)
>>> + rpcb_users++;
>>> + cnt = rpcb_users;
>>> + spin_unlock(&rpcb_clnt_lock);
>>> +
>>> + return cnt;
>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> + int shutdown;
>>> +
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (--rpcb_users == 0) {
>>> + rpcb_local_clnt = NULL;
>>> + rpcb_local_clnt4 = NULL;
>>> + }
>>
>> In the function below, you mention that the above pointers are
>> protected by rpcb_create_local_mutex, but it looks like they get reset
>> here without that being held?
>>
>
> Assigning of them is protected by rpcb_create_local_mutex.
> Dereferencing of them is protected by rpcb_clnt_lock.
Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them.
>
>> Might it be simpler to just protect rpcb_users with the
>> rpcb_create_local_mutex and ensure that it's held whenever you call one
>> of these routines? None of these are codepaths are particularly hot.
>>
>
> I just inherited this lock-mutex logic.
> Actually, you right. This codepaths are used rarely.
> But are use sure, that we need to remove this "speed-up" logic if we take into account that it was here already?
>
>>> + shutdown = !rpcb_users;
>>> + spin_unlock(&rpcb_clnt_lock);
>>> +
>>> + if (shutdown) {
>>> + /*
>>> + * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>>> + */
>>> + if (clnt4)
>>> + rpc_shutdown_client(clnt4);
>>> + if (clnt)
>>> + rpc_shutdown_client(clnt);
>>> + }
>>> + return;
>>> +}
>>> +
>>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
>>> +{
>>> + /* Protected by rpcb_create_local_mutex */
>>> + rpcb_local_clnt = clnt;
>>> + rpcb_local_clnt4 = clnt4;
>>> + rpcb_users++;
>>> + dprintk("RPC: created new rpcb local clients (rpcb_local_clnt: "
>>> + "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>>> + rpcb_local_clnt4);
>>> +}
>>> +
>>> /*
>>> * Returns zero on success, otherwise a negative errno value
>>> * is returned.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 15:03 UTC (permalink / raw)
To: Myklebust, Trond
Cc: Schumaker, Bryan, linux-nfs@vger.kernel.org, Pavel Emelianov,
neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <2E1EB2CF9ED1CB4AA966F0EB76EAB4430B47FD2B@SACMVEXC2-PRD.hq.netapp.com>
20.09.2011 18:38, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>> Sent: Tuesday, September 20, 2011 10:35 AM
>> To: Myklebust, Trond
>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> 20.09.2011 18:14, Myklebust, Trond пишет:
>>
>>>>>
>>>>> Doesn't it need to be protected by rpcb_clnt_lock too?
>>>>>
>>>>
>>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
>>>> one will change rpcb_users since it's zero. If it's non zero - we
>>>> willn't get to rpcb_set_local().
>>>
>>> OK, so you are saying that the rpcb_users++ below could be replaced by
>> rpcb_users=1?
>>>
>>
>> Yes, you right.
>>
>>> In that case, don't you need a smp_wmb() between the setting of
>> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
>> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>>>
>>
>> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
>> doesn't it?
>
> Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.
>
Yep, now I understand what are you talking about.
Will fix both places (set and put).
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Jeff Layton @ 2011-09-20 15:11 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <4E78A6A1.1000800@parallels.com>
On Tue, 20 Sep 2011 18:43:45 +0400
Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:
> 20.09.2011 18:24, Jeff Layton пишет:
> > On Tue, 20 Sep 2011 17:49:27 +0400
> > Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:
> >
> >> v5: fixed races with rpcb_users in rpcb_get_local()
> >>
> >> This helpers will be used for dynamical creation and destruction of rpcbind
> >> clients.
> >> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> >> clients has been created already, then we just increase rpcb_users.
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
> >>
> >> ---
> >> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 files changed, 53 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> >> index e45d2fb..5f4a406 100644
> >> --- a/net/sunrpc/rpcb_clnt.c
> >> +++ b/net/sunrpc/rpcb_clnt.c
> >> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
> >> static struct rpc_clnt * rpcb_local_clnt;
> >> static struct rpc_clnt * rpcb_local_clnt4;
> >> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >> +unsigned int rpcb_users;
> >> +
> >> struct rpcbind_args {
> >> struct rpc_xprt * r_xprt;
> >> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
> >> kfree(map);
> >> }
> >> +static int rpcb_get_local(void)
> >> +{
> >> + int cnt;
> >> +
> >> + spin_lock(&rpcb_clnt_lock);
> >> + if (rpcb_users)
> >> + rpcb_users++;
> >> + cnt = rpcb_users;
> >> + spin_unlock(&rpcb_clnt_lock);
> >> +
> >> + return cnt;
> >> +}
> >> +
> >> +void rpcb_put_local(void)
> >> +{
> >> + struct rpc_clnt *clnt = rpcb_local_clnt;
> >> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >> + int shutdown;
> >> +
> >> + spin_lock(&rpcb_clnt_lock);
> >> + if (--rpcb_users == 0) {
> >> + rpcb_local_clnt = NULL;
> >> + rpcb_local_clnt4 = NULL;
> >> + }
> >
> > In the function below, you mention that the above pointers are
> > protected by rpcb_create_local_mutex, but it looks like they get reset
> > here without that being held?
> >
>
> Assigning of them is protected by rpcb_create_local_mutex.
> Dereferencing of them is protected by rpcb_clnt_lock.
>
That's probably a bug, but I haven't sat down to work through the logic.
> > Might it be simpler to just protect rpcb_users with the
> > rpcb_create_local_mutex and ensure that it's held whenever you call one
> > of these routines? None of these are codepaths are particularly hot.
> >
>
> I just inherited this lock-mutex logic.
> Actually, you right. This codepaths are used rarely.
> But are use sure, that we need to remove this "speed-up" logic if we take into
> account that it was here already?
>
There are many ways to do this...
In general, it's difficult to get locking right, especially when you
start mixing multiple locks on related resources. Personally, I'd go
with a simpler scheme here. There's not much value in protecting this
counter with a spinlock when the other parts need to be protected by a
mutex. If you do decide to do it with multiple locks, then please do
document in comments how the locking is expected to work.
An alternate scheme might be to consider doing this with krefs, but I
haven't really considered whether that idiom makes sense here.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 15:38 UTC (permalink / raw)
To: Bryan Schumaker
Cc: Jeff Layton, Trond.Myklebust@netapp.com,
linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bfields@fieldses.org, davem@davemloft.net
In-Reply-To: <4E78AA09.1000209@netapp.com>
20.09.2011 18:58, Bryan Schumaker пишет:
> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
>> 20.09.2011 18:24, Jeff Layton пишет:
>>> On Tue, 20 Sep 2011 17:49:27 +0400
>>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:
>>>
>>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>>
>>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>>> clients.
>>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>>> clients has been created already, then we just increase rpcb_users.
>>>>
>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>
>>>> ---
>>>> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 files changed, 53 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>> index e45d2fb..5f4a406 100644
>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>>>> static struct rpc_clnt * rpcb_local_clnt;
>>>> static struct rpc_clnt * rpcb_local_clnt4;
>>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>>> +unsigned int rpcb_users;
>>>> +
>>>> struct rpcbind_args {
>>>> struct rpc_xprt * r_xprt;
>>>> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>> kfree(map);
>>>> }
>>>> +static int rpcb_get_local(void)
>>>> +{
>>>> + int cnt;
>>>> +
>>>> + spin_lock(&rpcb_clnt_lock);
>>>> + if (rpcb_users)
>>>> + rpcb_users++;
>>>> + cnt = rpcb_users;
>>>> + spin_unlock(&rpcb_clnt_lock);
>>>> +
>>>> + return cnt;
>>>> +}
>>>> +
>>>> +void rpcb_put_local(void)
>>>> +{
>>>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>>> + int shutdown;
>>>> +
>>>> + spin_lock(&rpcb_clnt_lock);
>>>> + if (--rpcb_users == 0) {
>>>> + rpcb_local_clnt = NULL;
>>>> + rpcb_local_clnt4 = NULL;
>>>> + }
>>>
>>> In the function below, you mention that the above pointers are
>>> protected by rpcb_create_local_mutex, but it looks like they get reset
>>> here without that being held?
>>>
>>
>> Assigning of them is protected by rpcb_create_local_mutex.
>> Dereferencing of them is protected by rpcb_clnt_lock.
>
> Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them.
Probably I wasn't clear with my previous explanation.
Actually, we use only spinlock to make sure, that the pointers are valid when we
dereferencing them. Synchronization point here is rpcb_users counter.
IOW, we use these pointers only from svc code and only after service already
started. And with this patch-set we can be sure, that this pointers has been
created already to the point, when this service starts.
But when this counter is zero, we can experience races with assigning those
pointers. It takes a lot of time, so we use local mutex here instead of spinlock.
Have I answered your question?
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 15:58 UTC (permalink / raw)
To: Myklebust, Trond
Cc: Jeff Layton, linux-nfs@vger.kernel.org, Pavel Emelianov,
neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <2E1EB2CF9ED1CB4AA966F0EB76EAB4430B47FD2C@SACMVEXC2-PRD.hq.netapp.com>
20.09.2011 18:41, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Jeff Layton [mailto:jlayton@redhat.com]
>> Sent: Tuesday, September 20, 2011 10:25 AM
>> To: Stanislav Kinsbursky
>> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> On Tue, 20 Sep 2011 17:49:27 +0400
>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:
>>
>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>
>>> This helpers will be used for dynamical creation and destruction of
>>> rpcbind clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services.
> If
>>> rpcbind clients has been created already, then we just increase
> rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>> net/sunrpc/rpcb_clnt.c | 53
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
>>> e45d2fb..5f4a406 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>>> static struct rpc_clnt * rpcb_local_clnt;
>>> static struct rpc_clnt * rpcb_local_clnt4;
>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int rpcb_users;
>>> +
>>> struct rpcbind_args {
>>> struct rpc_xprt * r_xprt;
>>> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>> kfree(map);
>>> }
>>> +static int rpcb_get_local(void)
>>> +{
>>> + int cnt;
>>> +
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (rpcb_users)
>>> + rpcb_users++;
>>> + cnt = rpcb_users;
>>> + spin_unlock(&rpcb_clnt_lock);
>>> +
>>> + return cnt;
>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> + int shutdown;
>>> +
>>> + spin_lock(&rpcb_clnt_lock);
>>> + if (--rpcb_users == 0) {
>>> + rpcb_local_clnt = NULL;
>>> + rpcb_local_clnt4 = NULL;
>>> + }
>>
>> In the function below, you mention that the above pointers are
> protected by
>> rpcb_create_local_mutex, but it looks like they get reset here without
> that
>> being held?
>>
>> Might it be simpler to just protect rpcb_users with the
>> rpcb_create_local_mutex and ensure that it's held whenever you call
> one of
>> these routines? None of these are codepaths are particularly hot.
>
> Alternatively, if you do
>
> if (rpcb_users == 1) {
> rpcb_local_clnt = NULL;
> rpcb_local_clnt4 = NULL;
> smp_wmb();
> rpcb_users = 0;
> } else
> rpcb_users--;
>
> then the spinlock protection in rpbc_get_local() is still good enough to
> guarantee correctness.
I don't understand the idea of this code. It guarantees, that if rpcb_users ==
0, then rpcb_local_clnt == NULL and rpcb_local_clnt4 == NULL.
But we don't need such guarantee from my pow.
I.e. if rpcb_users == 0, then it means, that no services running right now.
For example, processes, destroying those clients, is running on CPU#0.
On CPU#1, for example, we have another process trying to get those clients and
waiting on spinlock. When this process will gain the spinlock, it will see 0
users, gain mutex and then try to create new clients. We still have no users on
this clients yet. And this process will just reassign whose rpcbind clients
pointers (and here we need memmory barrier for sure).
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Bryan Schumaker @ 2011-09-20 16:06 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Jeff Layton, Trond.Myklebust@netapp.com,
linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bfields@fieldses.org, davem@davemloft.net
In-Reply-To: <4E78B389.3000307@parallels.com>
On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote:
> 20.09.2011 18:58, Bryan Schumaker пишет:
>> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
>>> 20.09.2011 18:24, Jeff Layton пишет:
>>>> On Tue, 20 Sep 2011 17:49:27 +0400
>>>> Stanislav Kinsbursky<skinsbursky@parallels.com> wrote:
>>>>
>>>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>>>
>>>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>>>> clients.
>>>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>>>> clients has been created already, then we just increase rpcb_users.
>>>>>
>>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>>
>>>>> ---
>>>>> net/sunrpc/rpcb_clnt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 files changed, 53 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>>> index e45d2fb..5f4a406 100644
>>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>>> @@ -114,6 +114,9 @@ static struct rpc_program rpcb_program;
>>>>> static struct rpc_clnt * rpcb_local_clnt;
>>>>> static struct rpc_clnt * rpcb_local_clnt4;
>>>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>>>> +unsigned int rpcb_users;
>>>>> +
>>>>> struct rpcbind_args {
>>>>> struct rpc_xprt * r_xprt;
>>>>> @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>>> kfree(map);
>>>>> }
>>>>> +static int rpcb_get_local(void)
>>>>> +{
>>>>> + int cnt;
>>>>> +
>>>>> + spin_lock(&rpcb_clnt_lock);
>>>>> + if (rpcb_users)
>>>>> + rpcb_users++;
>>>>> + cnt = rpcb_users;
>>>>> + spin_unlock(&rpcb_clnt_lock);
>>>>> +
>>>>> + return cnt;
>>>>> +}
>>>>> +
>>>>> +void rpcb_put_local(void)
>>>>> +{
>>>>> + struct rpc_clnt *clnt = rpcb_local_clnt;
>>>>> + struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>>>> + int shutdown;
>>>>> +
>>>>> + spin_lock(&rpcb_clnt_lock);
>>>>> + if (--rpcb_users == 0) {
>>>>> + rpcb_local_clnt = NULL;
>>>>> + rpcb_local_clnt4 = NULL;
>>>>> + }
>>>>
>>>> In the function below, you mention that the above pointers are
>>>> protected by rpcb_create_local_mutex, but it looks like they get reset
>>>> here without that being held?
>>>>
>>>
>>> Assigning of them is protected by rpcb_create_local_mutex.
>>> Dereferencing of them is protected by rpcb_clnt_lock.
>>
>> Shouldn't you be using the same lock for assigning and dereferencing? Otherwise one thread could change these variables while another is using them.
>
> Probably I wasn't clear with my previous explanation.
> Actually, we use only spinlock to make sure, that the pointers are valid when we dereferencing them. Synchronization point here is rpcb_users counter.
> IOW, we use these pointers only from svc code and only after service already started. And with this patch-set we can be sure, that this pointers has been created already to the point, when this service starts.
>
> But when this counter is zero, we can experience races with assigning those pointers. It takes a lot of time, so we use local mutex here instead of spinlock.
>
> Have I answered your question?
I think I understand now. Thanks!
>
^ permalink raw reply
* Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 16:20 UTC (permalink / raw)
To: Jeff Layton
Cc: Trond.Myklebust@netapp.com, linux-nfs@vger.kernel.org,
Pavel Emelianov, neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <20110920111112.3f07cd6e@tlielax.poochiereds.net>
20.09.2011 19:11, Jeff Layton пишет:
>
> In general, it's difficult to get locking right, especially when you
> start mixing multiple locks on related resources. Personally, I'd go
> with a simpler scheme here. There's not much value in protecting this
> counter with a spinlock when the other parts need to be protected by a
> mutex. If you do decide to do it with multiple locks, then please do
> document in comments how the locking is expected to work.
>
> An alternate scheme might be to consider doing this with krefs, but I
> haven't really considered whether that idiom makes sense here.
>
Jeff, please, have a look at my answer to Bryan Schumaker.
Does it allay your apprehensions?
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Stanislav Kinsbursky @ 2011-09-20 16:20 UTC (permalink / raw)
To: Myklebust, Trond
Cc: Schumaker, Bryan, linux-nfs@vger.kernel.org, Pavel Emelianov,
neilb@suse.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bfields@fieldses.org,
davem@davemloft.net
In-Reply-To: <2E1EB2CF9ED1CB4AA966F0EB76EAB4430B47FD2B@SACMVEXC2-PRD.hq.netapp.com>
20.09.2011 18:38, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>> Sent: Tuesday, September 20, 2011 10:35 AM
>> To: Myklebust, Trond
>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> 20.09.2011 18:14, Myklebust, Trond пишет:
>>
>>>>>
>>>>> Doesn't it need to be protected by rpcb_clnt_lock too?
>>>>>
>>>>
>>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
>>>> one will change rpcb_users since it's zero. If it's non zero - we
>>>> willn't get to rpcb_set_local().
>>>
>>> OK, so you are saying that the rpcb_users++ below could be replaced by
>> rpcb_users=1?
>>>
>>
>> Yes, you right.
>>
>>> In that case, don't you need a smp_wmb() between the setting of
>> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
>> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>>>
>>
>> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
>> doesn't it?
>
> Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.
>
Trond, I've thought again and realized, that even if these writes (rpcb_users
and rpbc_local_clnt/4) will be done in reversed order, then no barrier required
here.
Because if we have another process trying to create those clients (it can't use
them since it's not started yet) on other CPU, than it's waiting on creation
mutex. When it will gain the mutex, we will have required memory barrier for us.
Or I missed something in your explanation?
--
Best regards,
Stanislav Kinsbursky
^ permalink raw reply
* RE: unicast hash in ixgbe and ixgbevf
From: Rose, Gregory V @ 2011-09-20 16:51 UTC (permalink / raw)
To: frog1120@gmail.com, netdev@vger.kernel.org
In-Reply-To: <4E747E5B.4050207@gmail.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of J.Hwan.Kim
> Sent: Saturday, September 17, 2011 4:03 AM
> To: netdev@vger.kernel.org
> Subject: unicast hash in ixgbe and ixgbevf
>
> Hi, everyone
>
> I'm using intel ixgbe and ixgbevf driver and SR-IOV option.
>
> Is there any method for distributing the random IP packet stream
> into several VF queues evenly?
>
> For example, for each 4 VF queues,
> can each a quarter of the IP stream be distributed to each 4 VF queues?
>
> I checked the PFUTA register and tested it, but as far as I know,
> with the 4K bit hash table, the stream is directed to queues
> of which ROPE bit is set.
> So, the stream is duplicated to the queues of which ROPE bit is set.
> That is not what I want to do.
>
> How can I distribute the random IP packets to several VF queues
> without duplicating?
[Greg Rose]
Distribution of packets to different queues based upon IP address requires RSS which is not supported in our SR-IOV functions at this time.
Also, on the Intel 82599 and X540 adapters the VFs only support single queue operation. There are plans for multiple queue operation based upon RSS in future products, but there is not support for it now.
- Greg Rose
LAN Access Division
Intel Corp.
^ permalink raw reply
* Re: pull request: wireless-next 2011-09-19
From: David Miller @ 2011-09-20 16:57 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev
In-Reply-To: <20110920031929.GA2383@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Mon, 19 Sep 2011 23:19:29 -0400
> On Mon, Sep 19, 2011 at 05:11:24PM -0400, David Miller wrote:
>> From: "John W. Linville" <linville@tuxdriver.com>
>> Date: Mon, 19 Sep 2011 16:36:39 -0400
>>
>> > Here is another big batch of updates intended for 3.2 -- still
>> > clearing-out the backlog...
>> >
>> > This round includes lots of updates to ath9k, b43, iwlagn, and rt2x00.
>> > Some cleanups go to mac80211, along with a number of mesh-mode fixes
>> > from Javier.
>> >
>> > Please let me know if there are problems!
>>
>> What about the iwl build failure reported by Eric Dumazet?
>>
>> If that's not handled here, I want that fixed before we add even
>> more regressions :-)
>
> It's there. :-)
Ok, now tell me how I can pull from that ssh://infradead/ URL? :-)
^ permalink raw reply
* Re: pull request: wireless-next 2011-09-19
From: David Miller @ 2011-09-20 17:10 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110920141849.GD7800-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Tue, 20 Sep 2011 10:18:50 -0400
> This is also available at this URL -- sorry about the ssh URL (with
> an alias at that)!
>
> git://git.infradead.org/users/linville/wireless-next.git for-davem
Pulled, thanks John.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients
From: Myklebust, Trond @ 2011-09-20 17:13 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Schumaker, Bryan, linux-nfs, Pavel Emelianov, neilb, netdev,
linux-kernel, bfields, davem
In-Reply-To: <4E78BD5B.8090507@parallels.com>
> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 12:21 PM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
>
> 20.09.2011 18:38, Myklebust, Trond пишет:
> >> -----Original Message-----
> >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> >> Sent: Tuesday, September 20, 2011 10:35 AM
> >> To: Myklebust, Trond
> >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> bfields@fieldses.org; davem@davemloft.net
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> 20.09.2011 18:14, Myklebust, Trond пишет:
> >>
> >>>>>
> >>>>> Doesn't it need to be protected by rpcb_clnt_lock too?
> >>>>>
> >>>>
> >>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e.
> >>>> no one will change rpcb_users since it's zero. If it's non zero -
> >>>> we willn't get to rpcb_set_local().
> >>>
> >>> OK, so you are saying that the rpcb_users++ below could be replaced
> >>> by
> >> rpcb_users=1?
> >>>
> >>
> >> Yes, you right.
> >>
> >>> In that case, don't you need a smp_wmb() between the setting of
> >> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do
> >> you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
> >>>
> >>
> >> We check rpcb_users under spinlock. Gain spinlock forces memory
> >> barrier, doesn't it?
> >
> > Yes, and so you don't need an smp_rmb() on the reader side. However,
> you still need to ensure that the processor which _sets_ the rpcb_users and
> rpcb_local_clnt/4 actually writes them in the correct order.
> >
>
> Trond, I've thought again and realized, that even if these writes (rpcb_users
> and rpbc_local_clnt/4) will be done in reversed order, then no barrier
> required here.
> Because if we have another process trying to create those clients (it can't use
> them since it's not started yet) on other CPU, than it's waiting on creation
> mutex. When it will gain the mutex, we will have required memory barrier
> for us.
>
> Or I missed something in your explanation?
You need to ensure that if someone calls rpcb_get_local() and gets a positive result, then they can rely on rpcb_local_clnt/4 being non-zero. Without the write barrier, that is not the case.
Without that guarantee, you can't really ensure that rpcb_put_local() will work as expected either, since it will be possible to trigger the --rpcb_users == 0 case without shutting down rpcb_local_clnt/4 (because clnt/clnt4 == 0).
Cheers
Trond
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox