* 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 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: 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: 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 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: 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 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: 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 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 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: 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: 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: [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: [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: 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 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: 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 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: 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: Myklebust, Trond @ 2011-09-20 13:15 UTC (permalink / raw)
To: Schumaker, Bryan, Stanislav Kinsbursky
Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem
In-Reply-To: <4E788F8C.20103@netapp.com>
> -----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.
> > +}
> > +
> > +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?
> > + 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: Bryan Schumaker @ 2011-09-20 13:05 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Trond.Myklebust, linux-nfs, xemul, neilb, netdev, linux-kernel,
bfields, davem
In-Reply-To: <20110920101341.9861.51453.stgit@localhost6.localdomain6>
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?
> +}
> +
> +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.
>
> --
> 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 0/2] SUNRPC: pipefs virtualization
From: Stanislav Kinsbursky @ 2011-09-20 12:13 UTC (permalink / raw)
To: Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
Cc: 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: <20110912141633.6261.72180.stgit-bi+AKbBUZKagILUCTcTcHdKyNwTtLsGr@public.gmane.org>
Probably, holding mount point of network namespace is not a good solution.
There is another one. But I really hope for discussion of it.
RPC pipefs can be virtualized in the way like sysfs done.
But the main problem here is that currently we have only one RPC pipefs mount
point (superblock, root) for all.
New mount point can be created per every pipefs mount call. We can even have
some shared privates on dentries with the same name, like sysfs does (but I
don't see any good reason for this).
But with many mount points we have a problem with RPC pipes creation. I.e. we
have to find proper mount point for current network namespace, which is required
for vfs_path_lookup(), which is done prior to pipe creation.
And thus we have to store this per-netns mount points somewhere. It could be
some static variable. But we have to implement search function for them.
This approach can hide all pipefs virtualization from it's users. But it's still
look ugly from my pow.
Maybe, somebody have suggest better solution for this problem?
12.09.2011 18:19, Stanislav Kinsbursky пишет:
> This patch set is a part of further RPC layer virtualization and it's aim is to
> make RPC pipefs mount creation and destruction per network namespace context.
> It moves RPC pipefs internal data to sunrpc_net instance of network namespace
> context.
> With this patch set all calls to pipefs infrastructure are performed with
> "&init_net" except rpc_new_client() and rpc_setup_pipedir() functions (but they
> still passes pointer to "init_net" by current design).
> This "init_net" pointer will be replaced later with NFS virtualization.
>
> The following series consists of:
>
> ---
>
> Stanislav Kinsbursky (2):
> SUNRPC: make rpc pipefs mount per network namespace
> SUNRPC: RPC pipefs virtualization
>
>
> fs/nfs/blocklayout/blocklayout.c | 2 +-
> fs/nfs/cache_lib.c | 7 ++++---
> include/linux/sunrpc/rpc_pipe_fs.h | 4 ++--
> net/sunrpc/clnt.c | 8 ++++----
> net/sunrpc/netns.h | 3 +++
> net/sunrpc/rpc_pipe.c | 23 +++++++++++++++--------
> 6 files changed, 29 insertions(+), 18 deletions(-)
>
--
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
* I Need Your Assistance
From: Mr Ahmed Hassan @ 2011-09-20 11:53 UTC (permalink / raw)
Dear Beloved,
I Mr Ahmed Hassan,a Business merchant who is diagnosed with
"Esophageal Cancer"
which
defiled all forms of medical treatment and i have cash deposit of Twenty One
million dollars ($21,000,000,00) that I have with a finance Company abroad,i
need someone to stand as a benefactor,if you are interested indicate your
interest.and i will give you the full details
^ permalink raw reply
* [PATCH net-next 3/3] dp83640: add time stamp insertion for sync messages
From: Richard Cochran @ 2011-09-20 11:43 UTC (permalink / raw)
To: netdev; +Cc: David Miller
In-Reply-To: <cover.1316518332.git.richard.cochran@omicron.at>
This commit adds one step support to the phyter. When enabled, the
hardware does not provide time stamps for transmitted sync messages but
instead inserts the stamp into the outgoing packet.
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
drivers/net/phy/dp83640.c | 70 ++++++++++++++++++++++++++++++++++++--------
1 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index d3c6a2e..c588a16 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -761,6 +761,41 @@ static void decode_status_frame(struct dp83640_private *dp83640,
}
}
+static int is_sync(struct sk_buff *skb, int type)
+{
+ u8 *data = skb->data, *msgtype;
+ unsigned int offset = 0;
+
+ switch (type) {
+ case PTP_CLASS_V1_IPV4:
+ case PTP_CLASS_V2_IPV4:
+ offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
+ break;
+ case PTP_CLASS_V1_IPV6:
+ case PTP_CLASS_V2_IPV6:
+ offset = OFF_PTP6;
+ break;
+ case PTP_CLASS_V2_L2:
+ offset = ETH_HLEN;
+ break;
+ case PTP_CLASS_V2_VLAN:
+ offset = ETH_HLEN + VLAN_HLEN;
+ break;
+ default:
+ return 0;
+ }
+
+ if (type & PTP_CLASS_V1)
+ offset += OFF_PTP_CONTROL;
+
+ if (skb->len < offset + 1)
+ return 0;
+
+ msgtype = data + offset;
+
+ return (*msgtype & 0xf) == 0;
+}
+
static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
{
u16 *seqid;
@@ -1010,16 +1045,10 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
if (cfg.flags) /* reserved for future extensions */
return -EINVAL;
- switch (cfg.tx_type) {
- case HWTSTAMP_TX_OFF:
- dp83640->hwts_tx_en = 0;
- break;
- case HWTSTAMP_TX_ON:
- dp83640->hwts_tx_en = 1;
- break;
- default:
+ if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
return -ERANGE;
- }
+
+ dp83640->hwts_tx_en = cfg.tx_type;
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
@@ -1074,6 +1103,9 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
if (dp83640->hwts_tx_en)
txcfg0 |= TX_TS_EN;
+ if (dp83640->hwts_tx_en == HWTSTAMP_TX_ONESTEP_SYNC)
+ txcfg0 |= SYNC_1STEP | CHK_1STEP;
+
if (dp83640->hwts_rx_en)
rxcfg0 |= RX_TS_EN;
@@ -1156,12 +1188,24 @@ static void dp83640_txtstamp(struct phy_device *phydev,
{
struct dp83640_private *dp83640 = phydev->priv;
- if (!dp83640->hwts_tx_en) {
+ switch (dp83640->hwts_tx_en) {
+
+ case HWTSTAMP_TX_ONESTEP_SYNC:
+ if (is_sync(skb, type)) {
+ kfree_skb(skb);
+ return;
+ }
+ /* fall through */
+ case HWTSTAMP_TX_ON:
+ skb_queue_tail(&dp83640->tx_queue, skb);
+ schedule_work(&dp83640->ts_work);
+ break;
+
+ case HWTSTAMP_TX_OFF:
+ default:
kfree_skb(skb);
- return;
+ break;
}
- skb_queue_tail(&dp83640->tx_queue, skb);
- schedule_work(&dp83640->ts_work);
}
static struct phy_driver dp83640_driver = {
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 2/3] net: introduce ptp one step time stamp mode for sync packets
From: Richard Cochran @ 2011-09-20 11:43 UTC (permalink / raw)
To: netdev; +Cc: David Miller
In-Reply-To: <cover.1316518332.git.richard.cochran@omicron.at>
The IEEE 1588 standard (PTP) has a provision for a "one step" mode, where
time stamps on outgoing event packets are inserted into the packet by the
hardware on the fly. This patch adds a new flag for the SIOCSHWTSTAMP
ioctl that lets user space programs request this mode.
Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
include/linux/net_tstamp.h | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index a3b8546..3df0984 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -60,6 +60,15 @@ enum {
* before sending the packet.
*/
HWTSTAMP_TX_ON,
+
+ /*
+ * Enables time stamping for outgoing packets just as
+ * HWTSTAMP_TX_ON does, but also enables time stamp insertion
+ * directly into Sync packets. In this case, transmitted Sync
+ * packets will not received a time stamp via the socket error
+ * queue.
+ */
+ HWTSTAMP_TX_ONESTEP_SYNC,
};
/* possible values for hwtstamp_config->rx_filter */
--
1.7.2.5
^ permalink raw reply related
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