* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Lai Jiangshan @ 2013-02-26 12:59 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Michel Lespinasse, linux-doc, peterz, fweisbec, linux-kernel,
namhyung, mingo, linux-arch, linux, xiaoguangrong, wangyun,
paulmck, nikunj, linux-pm, rusty, rostedt, rjw, vincent.guittot,
tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <512C7A38.8060906@linux.vnet.ibm.com>
On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 02/26/2013 05:47 AM, Lai Jiangshan wrote:
>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>> Hi Lai,
>>>
>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>>>> Hi, Srivatsa,
>>>>
>>>> The target of the whole patchset is nice for me.
>>>
>>> Cool! Thanks :-)
>>>
> [...]
>>>> I wrote an untested draft here.
>>>>
>>>> Thanks,
>>>> Lai
>>>>
>>>> PS: Some HA tools(I'm writing one) which takes checkpoints of
>>>> virtual-machines frequently, I guess this patchset can speedup the
>>>> tools.
>>>>
>>>> From 01db542693a1b7fc6f9ece45d57cb529d9be5b66 Mon Sep 17 00:00:00 2001
>>>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>>>> Date: Mon, 25 Feb 2013 23:14:27 +0800
>>>> Subject: [PATCH] lglock: add read-preference local-global rwlock
>>>>
>>>> locality via lglock(trylock)
>>>> read-preference read-write-lock via fallback rwlock_t
>>>>
>>>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>>>> ---
>>>> include/linux/lglock.h | 31 +++++++++++++++++++++++++++++++
>>>> kernel/lglock.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 76 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/linux/lglock.h b/include/linux/lglock.h
>>>> index 0d24e93..30fe887 100644
>>>> --- a/include/linux/lglock.h
>>>> +++ b/include/linux/lglock.h
>>>> @@ -67,4 +67,35 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu);
>>>> void lg_global_lock(struct lglock *lg);
>>>> void lg_global_unlock(struct lglock *lg);
>>>>
>>>> +struct lgrwlock {
>>>> + unsigned long __percpu *fallback_reader_refcnt;
>>>> + struct lglock lglock;
>>>> + rwlock_t fallback_rwlock;
>>>> +};
>>>> +
>>>> +#define DEFINE_LGRWLOCK(name) \
>>>> + static DEFINE_PER_CPU(arch_spinlock_t, name ## _lock) \
>>>> + = __ARCH_SPIN_LOCK_UNLOCKED; \
>>>> + static DEFINE_PER_CPU(unsigned long, name ## _refcnt); \
>>>> + struct lgrwlock name = { \
>>>> + .fallback_reader_refcnt = &name ## _refcnt, \
>>>> + .lglock = { .lock = &name ## _lock } }
>>>> +
>>>> +#define DEFINE_STATIC_LGRWLOCK(name) \
>>>> + static DEFINE_PER_CPU(arch_spinlock_t, name ## _lock) \
>>>> + = __ARCH_SPIN_LOCK_UNLOCKED; \
>>>> + static DEFINE_PER_CPU(unsigned long, name ## _refcnt); \
>>>> + static struct lgrwlock name = { \
>>>> + .fallback_reader_refcnt = &name ## _refcnt, \
>>>> + .lglock = { .lock = &name ## _lock } }
>>>> +
>>>> +static inline void lg_rwlock_init(struct lgrwlock *lgrw, char *name)
>>>> +{
>>>> + lg_lock_init(&lgrw->lglock, name);
>>>> +}
>>>> +
>>>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw);
>>>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw);
>>>> +void lg_rwlock_global_write_lock(struct lgrwlock *lgrw);
>>>> +void lg_rwlock_global_write_unlock(struct lgrwlock *lgrw);
>>>> #endif
>>>> diff --git a/kernel/lglock.c b/kernel/lglock.c
>>>> index 6535a66..463543a 100644
>>>> --- a/kernel/lglock.c
>>>> +++ b/kernel/lglock.c
>>>> @@ -87,3 +87,48 @@ void lg_global_unlock(struct lglock *lg)
>>>> preempt_enable();
>>>> }
>>>> EXPORT_SYMBOL(lg_global_unlock);
>>>> +
>>>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw)
>>>> +{
>>>> + struct lglock *lg = &lgrw->lglock;
>>>> +
>>>> + preempt_disable();
>>>> + if (likely(!__this_cpu_read(*lgrw->fallback_reader_refcnt))) {
>>>> + if (likely(arch_spin_trylock(this_cpu_ptr(lg->lock)))) {
>>>> + rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
>>>> + return;
>>>> + }
>>>> + read_lock(&lgrw->fallback_rwlock);
>>>> + }
>>>> +
>>>> + __this_cpu_inc(*lgrw->fallback_reader_refcnt);
>>>> +}
>>>> +EXPORT_SYMBOL(lg_rwlock_local_read_lock);
>>>> +
>>>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
>>>> +{
>>>> + if (likely(!__this_cpu_read(*lgrw->fallback_reader_refcnt))) {
>>>> + lg_local_unlock(&lgrw->lglock);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (!__this_cpu_dec_return(*lgrw->fallback_reader_refcnt))
>>>> + read_unlock(&lgrw->fallback_rwlock);
>>>> +
>>>> + preempt_enable();
>>>> +}
>>>> +EXPORT_SYMBOL(lg_rwlock_local_read_unlock);
>>>> +
>>>
>>> If I read the code above correctly, all you are doing is implementing a
>>> recursive reader-side primitive (ie., allowing the reader to call these
>>> functions recursively, without resulting in a self-deadlock).
>>>
>>> But the thing is, making the reader-side recursive is the least of our
>>> problems! Our main challenge is to make the locking extremely flexible
>>> and also safe-guard it against circular-locking-dependencies and deadlocks.
>>> Please take a look at the changelog of patch 1 - it explains the situation
>>> with an example.
>>
>>
>> My lock fixes your requirements(I read patch 1-6 before I sent). In
>> readsite, lglock 's lock is token via trylock, the lglock doesn't
>> contribute to deadlocks, we can consider it doesn't exist when we find
>> deadlock from it. And global fallback rwlock doesn't result to
>> deadlocks because it is read-preference(you need to inc the
>> fallback_reader_refcnt inside the cpu-hotplug write-side, I don't do
>> it in generic lgrwlock)
>>
>
> Ah, since you hadn't mentioned the increment at the writer-side in your
> previous email, I had missed the bigger picture of what you were trying
> to achieve.
>
>>
>> If lg_rwlock_local_read_lock() spins, which means
>> lg_rwlock_local_read_lock() spins on fallback_rwlock, and which means
>> lg_rwlock_global_write_lock() took the lgrwlock successfully and
>> return, and which means lg_rwlock_local_read_lock() will stop spinning
>> when the write side finished.
>>
>
> Unfortunately, I see quite a few issues with the code above. IIUC, the
> writer and the reader both increment the same counters. So how will the
> unlock() code in the reader path know when to unlock which of the locks?
The same as your code, the reader(which nested in write C.S.) just dec
the counters.
> (The counter-dropping-to-zero logic is not safe, since it can be updated
> due to different reasons). And now that I look at it again, in the absence
> of the writer, the reader is allowed to be recursive at the heavy cost of
> taking the global rwlock for read, every 2nd time you nest (because the
> spinlock is non-recursive).
(I did not understand your comments of this part)
nested reader is considered seldom. But if N(>=2) nested readers happen,
the overhead is:
1 spin_try_lock() + 1 read_lock() + (N-1) __this_cpu_inc()
> Also, this lg_rwlock implementation uses 3
> different data-structures - a per-cpu spinlock, a global rwlock and
> a per-cpu refcnt, and its not immediately apparent why you need those many
> or even those many varieties.
data-structures is the same as yours.
fallback_reader_refcnt <--> reader_refcnt
per-cpu spinlock <--> write_signal
fallback_rwlock <---> global_rwlock
> Also I see that this doesn't handle the
> case of interrupt-handlers also being readers.
handled. nested reader will see the ref or take the fallback_rwlock
>
> IMHO, the per-cpu rwlock scheme that I have implemented in this patchset
> has a clean, understandable design and just enough data-structures/locks
> to achieve its goal and has several optimizations (like reducing the
> interrupts-disabled time etc) included - all in a very straight-forward
> manner. Since this is non-trivial, IMHO, starting from a clean slate is
> actually better than trying to retrofit the logic into some locking scheme
> which we actively want to avoid (and hence effectively we aren't even
> borrowing anything from!).
>
> To summarize, if you are just pointing out that we can implement the same
> logic by altering lglocks, then sure, I acknowledge the possibility.
> However, I don't think doing that actually makes it better; it either
> convolutes the logic unnecessarily, or ends up looking _very_ similar to
> the implementation in this patchset, from what I can see.
>
> Regards,
> Srivatsa S. Bhat
>
^ permalink raw reply
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Lai Jiangshan @ 2013-02-26 13:34 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Michel Lespinasse, linux-doc, peterz, fweisbec, linux-kernel,
namhyung, mingo, linux-arch, linux, xiaoguangrong, wangyun,
paulmck, nikunj, linux-pm, rusty, rostedt, rjw, vincent.guittot,
tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm, linuxppc-dev,
Lai Jiangshan
In-Reply-To: <512BBAD8.8010006@linux.vnet.ibm.com>
On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Hi Lai,
>
> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>> Hi, Srivatsa,
>>
>> The target of the whole patchset is nice for me.
>
> Cool! Thanks :-)
>
>> A question: How did you find out the such usages of
>> "preempt_disable()" and convert them? did all are converted?
>>
>
> Well, I scanned through the source tree for usages which implicitly
> disabled CPU offline and converted them over.
How do you scan? could you show the way you scan the source tree.
I can follow your instructions for double checking.
> Its not limited to uses
> of preempt_disable() alone - even spin_locks, rwlocks, local_irq_disable()
> etc also help disable CPU offline. So I tried to dig out all such uses
> and converted them. However, since the merge window is open, a lot of
> new code is flowing into the tree. So I'll have to rescan the tree to
> see if there are any more places to convert.
I remember some code has such assumption:
preempt_disable() (or something else)
//the code assume that the cpu_online_map can't be changed.
preempt_enable()
It is very hard to find out all such kinds of assumptions and fixes them.
(I notice your code mainly fixes code around send_xxxx())
>
>> And I think the lock is too complex and reinvent the wheel, why don't
>> you reuse the lglock?
>
> lglocks? No way! ;-) See below...
>
>> I wrote an untested draft here.
>>
>> Thanks,
>> Lai
>>
>> PS: Some HA tools(I'm writing one) which takes checkpoints of
>> virtual-machines frequently, I guess this patchset can speedup the
>> tools.
>>
>> From 01db542693a1b7fc6f9ece45d57cb529d9be5b66 Mon Sep 17 00:00:00 2001
>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Date: Mon, 25 Feb 2013 23:14:27 +0800
>> Subject: [PATCH] lglock: add read-preference local-global rwlock
>>
>> locality via lglock(trylock)
>> read-preference read-write-lock via fallback rwlock_t
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>> include/linux/lglock.h | 31 +++++++++++++++++++++++++++++++
>> kernel/lglock.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 76 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/lglock.h b/include/linux/lglock.h
>> index 0d24e93..30fe887 100644
>> --- a/include/linux/lglock.h
>> +++ b/include/linux/lglock.h
>> @@ -67,4 +67,35 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu);
>> void lg_global_lock(struct lglock *lg);
>> void lg_global_unlock(struct lglock *lg);
>>
>> +struct lgrwlock {
>> + unsigned long __percpu *fallback_reader_refcnt;
>> + struct lglock lglock;
>> + rwlock_t fallback_rwlock;
>> +};
>> +
>> +#define DEFINE_LGRWLOCK(name) \
>> + static DEFINE_PER_CPU(arch_spinlock_t, name ## _lock) \
>> + = __ARCH_SPIN_LOCK_UNLOCKED; \
>> + static DEFINE_PER_CPU(unsigned long, name ## _refcnt); \
>> + struct lgrwlock name = { \
>> + .fallback_reader_refcnt = &name ## _refcnt, \
>> + .lglock = { .lock = &name ## _lock } }
>> +
>> +#define DEFINE_STATIC_LGRWLOCK(name) \
>> + static DEFINE_PER_CPU(arch_spinlock_t, name ## _lock) \
>> + = __ARCH_SPIN_LOCK_UNLOCKED; \
>> + static DEFINE_PER_CPU(unsigned long, name ## _refcnt); \
>> + static struct lgrwlock name = { \
>> + .fallback_reader_refcnt = &name ## _refcnt, \
>> + .lglock = { .lock = &name ## _lock } }
>> +
>> +static inline void lg_rwlock_init(struct lgrwlock *lgrw, char *name)
>> +{
>> + lg_lock_init(&lgrw->lglock, name);
>> +}
>> +
>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw);
>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw);
>> +void lg_rwlock_global_write_lock(struct lgrwlock *lgrw);
>> +void lg_rwlock_global_write_unlock(struct lgrwlock *lgrw);
>> #endif
>> diff --git a/kernel/lglock.c b/kernel/lglock.c
>> index 6535a66..463543a 100644
>> --- a/kernel/lglock.c
>> +++ b/kernel/lglock.c
>> @@ -87,3 +87,48 @@ void lg_global_unlock(struct lglock *lg)
>> preempt_enable();
>> }
>> EXPORT_SYMBOL(lg_global_unlock);
>> +
>> +void lg_rwlock_local_read_lock(struct lgrwlock *lgrw)
>> +{
>> + struct lglock *lg = &lgrw->lglock;
>> +
>> + preempt_disable();
>> + if (likely(!__this_cpu_read(*lgrw->fallback_reader_refcnt))) {
>> + if (likely(arch_spin_trylock(this_cpu_ptr(lg->lock)))) {
>> + rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
>> + return;
>> + }
>> + read_lock(&lgrw->fallback_rwlock);
>> + }
>> +
>> + __this_cpu_inc(*lgrw->fallback_reader_refcnt);
>> +}
>> +EXPORT_SYMBOL(lg_rwlock_local_read_lock);
>> +
>> +void lg_rwlock_local_read_unlock(struct lgrwlock *lgrw)
>> +{
>> + if (likely(!__this_cpu_read(*lgrw->fallback_reader_refcnt))) {
>> + lg_local_unlock(&lgrw->lglock);
>> + return;
>> + }
>> +
>> + if (!__this_cpu_dec_return(*lgrw->fallback_reader_refcnt))
>> + read_unlock(&lgrw->fallback_rwlock);
>> +
>> + preempt_enable();
>> +}
>> +EXPORT_SYMBOL(lg_rwlock_local_read_unlock);
>> +
>
> If I read the code above correctly, all you are doing is implementing a
> recursive reader-side primitive (ie., allowing the reader to call these
> functions recursively, without resulting in a self-deadlock).
>
> But the thing is, making the reader-side recursive is the least of our
> problems! Our main challenge is to make the locking extremely flexible
> and also safe-guard it against circular-locking-dependencies and deadlocks.
> Please take a look at the changelog of patch 1 - it explains the situation
> with an example.
>
>> +void lg_rwlock_global_write_lock(struct lgrwlock *lgrw)
>> +{
>> + lg_global_lock(&lgrw->lglock);
>
> This does a for-loop on all CPUs and takes their locks one-by-one. That's
> exactly what we want to prevent, because that is the _source_ of all our
> deadlock woes in this case. In the presence of perfect lock ordering
> guarantees, this wouldn't have been a problem (that's why lglocks are
> being used successfully elsewhere in the kernel). In the stop-machine()
> removal case, the over-flexibility of preempt_disable() forces us to provide
> an equally flexible locking alternative. Hence we can't use such per-cpu
> locking schemes.
>
> You might note that, for exactly this reason, I haven't actually used any
> per-cpu _locks_ in this synchronization scheme, though it is named as
> "per-cpu rwlocks". The only per-cpu component here are the refcounts, and
> we consciously avoid waiting/spinning on them (because then that would be
> equivalent to having per-cpu locks, which are deadlock-prone). We use
> global rwlocks to get the deadlock-safety that we need.
>
>> + write_lock(&lgrw->fallback_rwlock);
>> +}
>> +EXPORT_SYMBOL(lg_rwlock_global_write_lock);
>> +
>> +void lg_rwlock_global_write_unlock(struct lgrwlock *lgrw)
>> +{
>> + write_unlock(&lgrw->fallback_rwlock);
>> + lg_global_unlock(&lgrw->lglock);
>> +}
>> +EXPORT_SYMBOL(lg_rwlock_global_write_unlock);
>>
>
> Regards,
> Srivatsa S. Bhat
>
^ permalink raw reply
* [PATCH 0/2] netfilter fixes for net
From: pablo @ 2013-02-26 13:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
From: Pablo Neira Ayuso <pablo@netfilter.org>
Hi David,
The following patchset contains two bugfixes for netfilter/ipset via
Jozsef Kadlecsik, they are:
* Fix timeout corruption if sets are resized, by Josh Hunt.
* Fix bogus error report if the flag nomatch is set, from Jozsef.
You can pull these changes from:
git://1984.lsi.us.es/nf master
Thanks!
Josh Hunt (1):
netfilter: ipset: timeout values corrupted on set resize
Jozsef Kadlecsik (1):
netfilter: ipset: "Directory not empty" error message
include/linux/netfilter/ipset/ip_set_ahash.h | 4 +++-
net/netfilter/ipset/ip_set_core.c | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
--
1.7.10.4
^ permalink raw reply
* [PATCH 1/2] netfilter: ipset: timeout values corrupted on set resize
From: pablo @ 2013-02-26 13:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1361886320-9492-1-git-send-email-pablo@netfilter.org>
From: Josh Hunt <johunt@akamai.com>
If a resize is triggered on a set with timeouts enabled, the timeout
values will get corrupted when copying them to the new set. This occured
b/c the wrong timeout value is supplied to type_pf_elem_tadd().
This also adds simple debug statement similar to the one in type_pf_resize().
Signed-off-by: Josh Hunt <johunt@akamai.com>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
include/linux/netfilter/ipset/ip_set_ahash.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/netfilter/ipset/ip_set_ahash.h b/include/linux/netfilter/ipset/ip_set_ahash.h
index ef9acd3..01d25e6 100644
--- a/include/linux/netfilter/ipset/ip_set_ahash.h
+++ b/include/linux/netfilter/ipset/ip_set_ahash.h
@@ -854,6 +854,8 @@ type_pf_tresize(struct ip_set *set, bool retried)
retry:
ret = 0;
htable_bits++;
+ pr_debug("attempt to resize set %s from %u to %u, t %p\n",
+ set->name, orig->htable_bits, htable_bits, orig);
if (!htable_bits) {
/* In case we have plenty of memory :-) */
pr_warning("Cannot increase the hashsize of set %s further\n",
@@ -873,7 +875,7 @@ retry:
data = ahash_tdata(n, j);
m = hbucket(t, HKEY(data, h->initval, htable_bits));
ret = type_pf_elem_tadd(m, data, AHASH_MAX(h), 0,
- type_pf_data_timeout(data));
+ ip_set_timeout_get(type_pf_data_timeout(data)));
if (ret < 0) {
read_unlock_bh(&set->lock);
ahash_destroy(t);
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/2] netfilter: ipset: "Directory not empty" error message
From: pablo @ 2013-02-26 13:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1361886320-9492-1-git-send-email-pablo@netfilter.org>
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
When an entry flagged with "nomatch" was tested by ipset, it
returned the error message "Kernel error received:
Directory not empty" instead of "<element> is NOT in set <setname>"
(reported by John Brendler).
The internal error code was not properly transformed before returning
to userspace, fixed.
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
net/netfilter/ipset/ip_set_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 6d6d8f2..38ca630 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1470,7 +1470,8 @@ ip_set_utest(struct sock *ctnl, struct sk_buff *skb,
if (ret == -EAGAIN)
ret = 1;
- return ret < 0 ? ret : ret > 0 ? 0 : -IPSET_ERR_EXIST;
+ return (ret < 0 && ret != -ENOTEMPTY) ? ret :
+ ret > 0 ? 0 : -IPSET_ERR_EXIST;
}
/* Get headed data of a set */
--
1.7.10.4
^ permalink raw reply related
* [PATCH] drivers: net: ethernet: cpsw: consider number of slaves in interation
From: Daniel Mack @ 2013-02-26 14:06 UTC (permalink / raw)
To: netdev; +Cc: Daniel Mack, Mugunthan V N, David S. Miller
Make cpsw_add_default_vlan() look at the actual number of slaves for its
iteration, so boards with less than 2 slaves don't ooops at boot.
Signed-off-by: Daniel Mack <zonque@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: David S. Miller <davem@davemloft.net>
---
drivers/net/ethernet/ti/cpsw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 7e93df6..01ffbc4 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -731,7 +731,7 @@ static inline void cpsw_add_default_vlan(struct cpsw_priv *priv)
writel(vlan, &priv->host_port_regs->port_vlan);
- for (i = 0; i < 2; i++)
+ for (i = 0; i < priv->data.slaves; i++)
slave_write(priv->slaves + i, vlan, reg);
cpsw_ale_add_vlan(priv->ale, vlan, ALE_ALL_PORTS << port,
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Lai Jiangshan @ 2013-02-26 14:17 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: tglx, peterz, tj, oleg, paulmck, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel, walken, vincent.guittot
In-Reply-To: <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com>
On Mon, Feb 18, 2013 at 8:38 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
> lock-ordering related problems (unlike per-cpu locks). However, global
> rwlocks lead to unnecessary cache-line bouncing even when there are no
> writers present, which can slow down the system needlessly.
per-CPU rwlocks(yours and mine) are the exactly same as rwlock_t in
the view of lock dependency(except reader-C.S. can be nested in
writer-C.S.)
so they can deadlock in this order:
spin_lock(some_lock); percpu_write_lock_irqsave()
case CPU_DYING
percpu_read_lock_irqsafe(); <---deadlock---> spin_lock(some_lock);
The lockdep can find out such dependency, but we must try our best to
find out them before merge the patchset to mainline. We can review
all the code of cpu_disable() and CPU_DYING and fix this kinds of lock
dependency, but it is not easy thing, it may be a long term project.
======
And if there is any CPU_DYING code takes no locks and do some
works(because they know they are called via stop_machine()) we need to
add that locking code back if there is such code.(I don't know whether
such code exist or not)
>
> Per-cpu counters can help solve the cache-line bouncing problem. So we
> actually use the best of both: per-cpu counters (no-waiting) at the reader
> side in the fast-path, and global rwlocks in the slowpath.
>
> [ Fastpath = no writer is active; Slowpath = a writer is active ]
>
> IOW, the readers just increment/decrement their per-cpu refcounts (disabling
> interrupts during the updates, if necessary) when no writer is active.
> When a writer becomes active, he signals all readers to switch to global
> rwlocks for the duration of his activity. The readers switch over when it
> is safe for them (ie., when they are about to start a fresh, non-nested
> read-side critical section) and start using (holding) the global rwlock for
> read in their subsequent critical sections.
>
> The writer waits for every existing reader to switch, and then acquires the
> global rwlock for write and enters his critical section. Later, the writer
> signals all readers that he is done, and that they can go back to using their
> per-cpu refcounts again.
>
> Note that the lock-safety (despite the per-cpu scheme) comes from the fact
> that the readers can *choose* _when_ to switch to rwlocks upon the writer's
> signal. And the readers don't wait on anybody based on the per-cpu counters.
> The only true synchronization that involves waiting at the reader-side in this
> scheme, is the one arising from the global rwlock, which is safe from circular
> locking dependency issues.
>
> Reader-writer locks and per-cpu counters are recursive, so they can be
> used in a nested fashion in the reader-path, which makes per-CPU rwlocks also
> recursive. Also, this design of switching the synchronization scheme ensures
> that you can safely nest and use these locks in a very flexible manner.
>
> I'm indebted to Michael Wang and Xiao Guangrong for their numerous thoughtful
> suggestions and ideas, which inspired and influenced many of the decisions in
> this as well as previous designs. Thanks a lot Michael and Xiao!
>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
> lib/percpu-rwlock.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/lib/percpu-rwlock.c b/lib/percpu-rwlock.c
> index f938096..edefdea 100644
> --- a/lib/percpu-rwlock.c
> +++ b/lib/percpu-rwlock.c
> @@ -27,6 +27,24 @@
> #include <linux/percpu-rwlock.h>
> #include <linux/errno.h>
>
> +#include <asm/processor.h>
> +
> +
> +#define reader_yet_to_switch(pcpu_rwlock, cpu) \
> + (ACCESS_ONCE(per_cpu_ptr((pcpu_rwlock)->rw_state, cpu)->reader_refcnt))
> +
> +#define reader_percpu_nesting_depth(pcpu_rwlock) \
> + (__this_cpu_read((pcpu_rwlock)->rw_state->reader_refcnt))
> +
> +#define reader_uses_percpu_refcnt(pcpu_rwlock) \
> + reader_percpu_nesting_depth(pcpu_rwlock)
> +
> +#define reader_nested_percpu(pcpu_rwlock) \
> + (reader_percpu_nesting_depth(pcpu_rwlock) > 1)
> +
> +#define writer_active(pcpu_rwlock) \
> + (__this_cpu_read((pcpu_rwlock)->rw_state->writer_signal))
> +
>
> int __percpu_init_rwlock(struct percpu_rwlock *pcpu_rwlock,
> const char *name, struct lock_class_key *rwlock_key)
> @@ -55,21 +73,138 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)
>
> void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)
> {
> - read_lock(&pcpu_rwlock->global_rwlock);
> + preempt_disable();
> +
> + /*
> + * Let the writer know that a reader is active, even before we choose
> + * our reader-side synchronization scheme.
> + */
> + this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt);
> +
> + /*
> + * If we are already using per-cpu refcounts, it is not safe to switch
> + * the synchronization scheme. So continue using the refcounts.
> + */
> + if (reader_nested_percpu(pcpu_rwlock))
> + return;
> +
> + /*
> + * The write to 'reader_refcnt' must be visible before we read
> + * 'writer_signal'.
> + */
> + smp_mb();
> +
> + if (likely(!writer_active(pcpu_rwlock))) {
> + goto out;
> + } else {
> + /* Writer is active, so switch to global rwlock. */
> + read_lock(&pcpu_rwlock->global_rwlock);
> +
> + /*
> + * We might have raced with a writer going inactive before we
> + * took the read-lock. So re-evaluate whether we still need to
> + * hold the rwlock or if we can switch back to per-cpu
> + * refcounts. (This also helps avoid heterogeneous nesting of
> + * readers).
> + */
> + if (writer_active(pcpu_rwlock)) {
> + /*
> + * The above writer_active() check can get reordered
> + * with this_cpu_dec() below, but this is OK, because
> + * holding the rwlock is conservative.
> + */
> + this_cpu_dec(pcpu_rwlock->rw_state->reader_refcnt);
> + } else {
> + read_unlock(&pcpu_rwlock->global_rwlock);
> + }
> + }
> +
> +out:
> + /* Prevent reordering of any subsequent reads/writes */
> + smp_mb();
> }
>
> void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
> {
> - read_unlock(&pcpu_rwlock->global_rwlock);
> + /*
> + * We never allow heterogeneous nesting of readers. So it is trivial
> + * to find out the kind of reader we are, and undo the operation
> + * done by our corresponding percpu_read_lock().
> + */
> +
> + /* Try to fast-path: a nested percpu reader is the simplest case */
> + if (reader_nested_percpu(pcpu_rwlock)) {
> + this_cpu_dec(pcpu_rwlock->rw_state->reader_refcnt);
> + preempt_enable();
> + return;
> + }
> +
> + /*
> + * Now we are left with only 2 options: a non-nested percpu reader,
> + * or a reader holding rwlock
> + */
> + if (reader_uses_percpu_refcnt(pcpu_rwlock)) {
> + /*
> + * Complete the critical section before decrementing the
> + * refcnt. We can optimize this away if we are a nested
> + * reader (the case above).
> + */
> + smp_mb();
> + this_cpu_dec(pcpu_rwlock->rw_state->reader_refcnt);
> + } else {
> + read_unlock(&pcpu_rwlock->global_rwlock);
> + }
> +
> + preempt_enable();
> }
>
> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
> {
> + unsigned int cpu;
> +
> + /*
> + * Tell all readers that a writer is becoming active, so that they
> + * start switching over to the global rwlock.
> + */
> + for_each_possible_cpu(cpu)
> + per_cpu_ptr(pcpu_rwlock->rw_state, cpu)->writer_signal = true;
> +
> + smp_mb();
> +
> + /*
> + * Wait for every reader to see the writer's signal and switch from
> + * percpu refcounts to global rwlock.
> + *
> + * If a reader is still using percpu refcounts, wait for him to switch.
> + * Else, we can safely go ahead, because either the reader has already
> + * switched over, or the next reader that comes along on that CPU will
> + * notice the writer's signal and will switch over to the rwlock.
> + */
> +
> + for_each_possible_cpu(cpu) {
> + while (reader_yet_to_switch(pcpu_rwlock, cpu))
> + cpu_relax();
> + }
> +
> + smp_mb(); /* Complete the wait-for-readers, before taking the lock */
> write_lock(&pcpu_rwlock->global_rwlock);
> }
>
> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
> {
> + unsigned int cpu;
> +
> + /* Complete the critical section before clearing ->writer_signal */
> + smp_mb();
> +
> + /*
> + * Inform all readers that we are done, so that they can switch back
> + * to their per-cpu refcounts. (We don't need to wait for them to
> + * see it).
> + */
> + for_each_possible_cpu(cpu)
> + per_cpu_ptr(pcpu_rwlock->rw_state, cpu)->writer_signal = false;
> +
> write_unlock(&pcpu_rwlock->global_rwlock);
> }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-26 14:22 UTC (permalink / raw)
To: Lai Jiangshan
Cc: Michel Lespinasse, linux-doc, peterz, fweisbec, linux-kernel,
namhyung, mingo, linux-arch, linux, xiaoguangrong, wangyun,
paulmck, nikunj, linux-pm, rusty, rostedt, rjw, vincent.guittot,
tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <CACvQF51RvhM8XNqu3-1OTQ=cZUZ=pAX+_zx0a29+W67N1X+BgQ@mail.gmail.com>
Hi Lai,
I'm really not convinced that piggy-backing on lglocks would help
us in any way. But still, let me try to address some of the points
you raised...
On 02/26/2013 06:29 PM, Lai Jiangshan wrote:
> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote:
>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>> Hi Lai,
>>>>
>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>>>>> Hi, Srivatsa,
>>>>>
>>>>> The target of the whole patchset is nice for me.
>>>>
>>>> Cool! Thanks :-)
>>>>
>> [...]
>>
>> Unfortunately, I see quite a few issues with the code above. IIUC, the
>> writer and the reader both increment the same counters. So how will the
>> unlock() code in the reader path know when to unlock which of the locks?
>
> The same as your code, the reader(which nested in write C.S.) just dec
> the counters.
And that works fine in my case because the writer and the reader update
_two_ _different_ counters. If both of them update the same counter, there
will be a semantic clash - an increment of the counter can either mean that
a new writer became active, or it can also indicate a nested reader. A decrement
can also similarly have 2 meanings. And thus it will be difficult to decide
the right action to take, based on the value of the counter.
>
>> (The counter-dropping-to-zero logic is not safe, since it can be updated
>> due to different reasons). And now that I look at it again, in the absence
>> of the writer, the reader is allowed to be recursive at the heavy cost of
>> taking the global rwlock for read, every 2nd time you nest (because the
>> spinlock is non-recursive).
>
> (I did not understand your comments of this part)
> nested reader is considered seldom.
No, nested readers can be _quite_ frequent. Because, potentially all users
of preempt_disable() are readers - and its well-known how frequently we
nest preempt_disable(). As a simple example, any atomic reader who calls
smp_call_function() will become a nested reader, because smp_call_function()
itself is a reader. So reader nesting is expected to be quite frequent.
> But if N(>=2) nested readers happen,
> the overhead is:
> 1 spin_try_lock() + 1 read_lock() + (N-1) __this_cpu_inc()
>
In my patch, its just this_cpu_inc(). Note that these are _very_ hot paths.
So every bit of optimization that you can add is worthwhile.
And your read_lock() is a _global_ lock - thus, it can lead to a lot of
cache-line bouncing. That's *exactly* why I have used per-cpu refcounts in
my synchronization scheme, to avoid taking the global rwlock as much as possible.
Another important point to note is that, the overhead we are talking about
here, exists even when _not_ performing hotplug. And its the replacement to
the super-fast preempt_disable(). So its extremely important to consciously
minimize this overhead - else we'll end up slowing down the system significantly.
>> Also, this lg_rwlock implementation uses 3
>> different data-structures - a per-cpu spinlock, a global rwlock and
>> a per-cpu refcnt, and its not immediately apparent why you need those many
>> or even those many varieties.
>
> data-structures is the same as yours.
> fallback_reader_refcnt <--> reader_refcnt
This has semantic problems, as noted above.
> per-cpu spinlock <--> write_signal
Acquire/release of (spin) lock is costlier than inc/dec of a counter, IIUC.
> fallback_rwlock <---> global_rwlock
>
>> Also I see that this doesn't handle the
>> case of interrupt-handlers also being readers.
>
> handled. nested reader will see the ref or take the fallback_rwlock
>
I'm not referring to simple nested readers here, but interrupt handlers who
can act as readers. For starters, the arch_spin_trylock() is not safe when
interrupt handlers can also run the same code, right? You'll need to save
and restore interrupts at critical points in the code. Also, the __foo()
variants used to read/update the counters are not interrupt-safe. And,
the unlock() code in the reader path is again going to be confused about
what to do when interrupt handlers interrupt regular readers, due to the
messed up refcount.
>>
>> IMHO, the per-cpu rwlock scheme that I have implemented in this patchset
>> has a clean, understandable design and just enough data-structures/locks
>> to achieve its goal and has several optimizations (like reducing the
>> interrupts-disabled time etc) included - all in a very straight-forward
>> manner. Since this is non-trivial, IMHO, starting from a clean slate is
>> actually better than trying to retrofit the logic into some locking scheme
>> which we actively want to avoid (and hence effectively we aren't even
>> borrowing anything from!).
>>
>> To summarize, if you are just pointing out that we can implement the same
>> logic by altering lglocks, then sure, I acknowledge the possibility.
>> However, I don't think doing that actually makes it better; it either
>> convolutes the logic unnecessarily, or ends up looking _very_ similar to
>> the implementation in this patchset, from what I can see.
>>
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* [PATCH 0/4] sctp: fix association hangs due to reassembly/ordering logic
From: Lee A. Roberts @ 2013-02-26 14:36 UTC (permalink / raw)
To: netdev; +Cc: lee.roberts
From: "Lee A. Roberts" <lee.roberts@hp.com>
This series of patches resolves several SCTP association hangs observed during
SCTP stress testing. Observable symptoms include communications hangs with
data being held in the association reassembly and/or lobby (ordering) queues.
Close examination of reassembly/ordering queues may show either duplicated
or missing packets.
Lee A. Roberts (4):
sctp: fix association hangs due to off-by-one errors in
sctp_tsnmap_grow()
sctp: fix association hangs due to reneging packets below the
cumulative TSN ACK point
sctp: fix association hangs due to errors when reneging events from
the ordering queue
sctp: fix association hangs due to partial delivery errors
net/sctp/tsnmap.c | 13 ++++----
net/sctp/ulpqueue.c | 87 +++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 78 insertions(+), 22 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH 1/4] sctp: fix association hangs due to off-by-one errors in sctp_tsnmap_grow()
From: Lee A. Roberts @ 2013-02-26 14:36 UTC (permalink / raw)
To: netdev; +Cc: lee.roberts
In-Reply-To: <1361889376-22171-1-git-send-email-lee.roberts@hp.com>
From: "Lee A. Roberts" <lee.roberts@hp.com>
Resolve SCTP association hangs observed during SCTP stress
testing. Observable symptoms include communications hangs
with data being held in the association lobby (ordering)
queue. Close examination of reassembly/ordering queues shows
duplicated packets.
In sctp_tsnmap_mark(), correct off-by-one error when calculating
size value for sctp_tsnmap_grow().
In sctp_tsnmap_grow(), correct off-by-one error when copying
and resizing the tsnmap. If max_tsn_seen is in the LSB of the
word, this bit can be lost, causing the corresponding packet
to be transmitted again and to be entered as a duplicate into
the SCTP reassembly/ordering queues. Change parameter name
from "gap" (zero-based index) to "size" (one-based) to enhance
code readability.
Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
net/sctp/tsnmap.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/sctp/tsnmap.c b/net/sctp/tsnmap.c
index 5f25e0c..396c451 100644
--- a/net/sctp/tsnmap.c
+++ b/net/sctp/tsnmap.c
@@ -51,7 +51,7 @@
static void sctp_tsnmap_update(struct sctp_tsnmap *map);
static void sctp_tsnmap_find_gap_ack(unsigned long *map, __u16 off,
__u16 len, __u16 *start, __u16 *end);
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap);
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size);
/* Initialize a block of memory as a tsnmap. */
struct sctp_tsnmap *sctp_tsnmap_init(struct sctp_tsnmap *map, __u16 len,
@@ -124,7 +124,7 @@ int sctp_tsnmap_mark(struct sctp_tsnmap *map, __u32 tsn,
gap = tsn - map->base_tsn;
- if (gap >= map->len && !sctp_tsnmap_grow(map, gap))
+ if (gap >= map->len && !sctp_tsnmap_grow(map, gap + 1))
return -ENOMEM;
if (!sctp_tsnmap_has_gap(map) && gap == 0) {
@@ -360,23 +360,24 @@ __u16 sctp_tsnmap_num_gabs(struct sctp_tsnmap *map,
return ngaps;
}
-static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 gap)
+static int sctp_tsnmap_grow(struct sctp_tsnmap *map, u16 size)
{
unsigned long *new;
unsigned long inc;
u16 len;
- if (gap >= SCTP_TSN_MAP_SIZE)
+ if (size > SCTP_TSN_MAP_SIZE)
return 0;
- inc = ALIGN((gap - map->len),BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
+ inc = ALIGN((size - map->len), BITS_PER_LONG) + SCTP_TSN_MAP_INCREMENT;
len = min_t(u16, map->len + inc, SCTP_TSN_MAP_SIZE);
new = kzalloc(len>>3, GFP_ATOMIC);
if (!new)
return 0;
- bitmap_copy(new, map->tsn_map, map->max_tsn_seen - map->base_tsn);
+ bitmap_copy(new, map->tsn_map,
+ map->max_tsn_seen - map->cumulative_tsn_ack_point);
kfree(map->tsn_map);
map->tsn_map = new;
map->len = len;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/4] sctp: fix association hangs due to reneging packets below the cumulative TSN ACK point
From: Lee A. Roberts @ 2013-02-26 14:36 UTC (permalink / raw)
To: netdev; +Cc: lee.roberts
In-Reply-To: <1361889376-22171-1-git-send-email-lee.roberts@hp.com>
From: "Lee A. Roberts" <lee.roberts@hp.com>
Resolve SCTP association hangs observed during SCTP stress
testing. Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues. Close examination of reassembly queue shows
missing packets.
In sctp_ulpq_renege_list(), do not renege packets below the
cumulative TSN ACK point.
Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
net/sctp/ulpqueue.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index ada1746..63afddc 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -969,11 +969,16 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
tsnmap = &ulpq->asoc->peer.tsn_map;
- while ((skb = __skb_dequeue_tail(list)) != NULL) {
- freed += skb_headlen(skb);
+ while ((skb = skb_peek_tail(list)) != NULL) {
event = sctp_skb2event(skb);
tsn = event->tsn;
+ /* Don't renege below the Cumulative TSN ACK Point. */
+ if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
+ break;
+
+ __skb_unlink(skb, list);
+ freed += skb_headlen(skb);
sctp_ulpevent_free(event);
sctp_tsnmap_renege(tsnmap, tsn);
if (freed >= needed)
--
1.7.9.5
^ permalink raw reply related
* [PATCH 3/4] sctp: fix association hangs due to errors when reneging events from the ordering queue
From: Lee A. Roberts @ 2013-02-26 14:36 UTC (permalink / raw)
To: netdev; +Cc: lee.roberts
In-Reply-To: <1361889376-22171-1-git-send-email-lee.roberts@hp.com>
From: "Lee A. Roberts" <lee.roberts@hp.com>
Resolve SCTP association hangs observed during SCTP stress
testing. Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues. Close examination of reassembly queue shows
missing packets.
In sctp_ulpq_renege_list(), events being reneged from the
ordering queue may correspond to multiple TSNs. Identify
all affected packets; sum freed space and renege from the
tsnmap.
Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
net/sctp/ulpqueue.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 63afddc..f221fbb 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -962,8 +962,8 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
struct sk_buff_head *list, __u16 needed)
{
__u16 freed = 0;
- __u32 tsn;
- struct sk_buff *skb;
+ __u32 tsn, last_tsn;
+ struct sk_buff *skb, *flist, *last;
struct sctp_ulpevent *event;
struct sctp_tsnmap *tsnmap;
@@ -977,10 +977,28 @@ static __u16 sctp_ulpq_renege_list(struct sctp_ulpq *ulpq,
if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
break;
- __skb_unlink(skb, list);
+ /* Events in ordering queue may have multiple fragments
+ * corresponding to additional TSNs. Sum the total
+ * freed space; find the last TSN.
+ */
freed += skb_headlen(skb);
+ flist = skb_shinfo(skb)->frag_list;
+ for (last = flist; flist; flist = flist->next) {
+ last = flist;
+ freed += skb_headlen(last);
+ }
+ if (last)
+ last_tsn = sctp_skb2event(last)->tsn;
+ else
+ last_tsn = tsn;
+
+ /* Unlink the event, then renege all applicable TSNs. */
+ __skb_unlink(skb, list);
sctp_ulpevent_free(event);
- sctp_tsnmap_renege(tsnmap, tsn);
+ while (TSN_lte(tsn, last_tsn)) {
+ sctp_tsnmap_renege(tsnmap, tsn);
+ tsn++;
+ }
if (freed >= needed)
return freed;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH 4/4] sctp: fix association hangs due to partial delivery errors
From: Lee A. Roberts @ 2013-02-26 14:36 UTC (permalink / raw)
To: netdev; +Cc: lee.roberts
In-Reply-To: <1361889376-22171-1-git-send-email-lee.roberts@hp.com>
From: "Lee A. Roberts" <lee.roberts@hp.com>
Resolve SCTP association hangs observed during SCTP stress
testing. Observable symptoms include communications hangs
with data being held in the association reassembly and/or lobby
(ordering) queues. Close examination of reassembly queue shows
missing packets.
In sctp_ulpq_tail_data(), use return values 0,1 to indicate whether
a complete event (with MSG_EOR set) was delivered. A return value
of -ENOMEM continues to indicate an out-of-memory condition was
encountered.
In sctp_ulpq_retrieve_partial() and sctp_ulpq_retrieve_first(),
correct message reassembly logic for SCTP partial delivery.
Change logic to ensure that as much data as possible is sent
with the initial partial delivery and that following partial
deliveries contain all available data.
In sctp_ulpq_partial_delivery(), attempt partial delivery only
if the data on the head of the reassembly queue is at or before
the cumulative TSN ACK point.
In sctp_ulpq_renege(), use the modified return values from
sctp_ulpq_tail_data() to choose whether to attempt partial
delivery or to attempt to drain the reassembly queue as a
means to reduce memory pressure. Remove call to
sctp_tsnmap_mark(), as this is handled correctly in call to
sctp_ulpq_tail_data().
Signed-off-by: Lee A. Roberts <lee.roberts@hp.com>
---
net/sctp/ulpqueue.c | 54 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index f221fbb..482e3ea 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -106,6 +106,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
{
struct sk_buff_head temp;
struct sctp_ulpevent *event;
+ int event_eor = 0;
/* Create an event from the incoming chunk. */
event = sctp_ulpevent_make_rcvmsg(chunk->asoc, chunk, gfp);
@@ -127,10 +128,12 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
/* Send event to the ULP. 'event' is the sctp_ulpevent for
* very first SKB on the 'temp' list.
*/
- if (event)
+ if (event) {
+ event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
sctp_ulpq_tail_event(ulpq, event);
+ }
- return 0;
+ return event_eor;
}
/* Add a new event for propagation to the ULP. */
@@ -540,14 +543,19 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_partial(struct sctp_ulpq *ulpq)
ctsn = cevent->tsn;
switch (cevent->msg_flags & SCTP_DATA_FRAG_MASK) {
+ case SCTP_DATA_FIRST_FRAG:
+ if (!first_frag)
+ return NULL;
+ goto done;
case SCTP_DATA_MIDDLE_FRAG:
if (!first_frag) {
first_frag = pos;
next_tsn = ctsn + 1;
last_frag = pos;
- } else if (next_tsn == ctsn)
+ } else if (next_tsn == ctsn) {
next_tsn++;
- else
+ last_frag = pos;
+ } else
goto done;
break;
case SCTP_DATA_LAST_FRAG:
@@ -651,6 +659,14 @@ static struct sctp_ulpevent *sctp_ulpq_retrieve_first(struct sctp_ulpq *ulpq)
} else
goto done;
break;
+
+ case SCTP_DATA_LAST_FRAG:
+ if (!first_frag)
+ return NULL;
+ else
+ goto done;
+ break;
+
default:
return NULL;
}
@@ -1025,16 +1041,28 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
struct sctp_ulpevent *event;
struct sctp_association *asoc;
struct sctp_sock *sp;
+ __u32 ctsn;
+ struct sk_buff *skb;
asoc = ulpq->asoc;
sp = sctp_sk(asoc->base.sk);
/* If the association is already in Partial Delivery mode
- * we have noting to do.
+ * we have nothing to do.
*/
if (ulpq->pd_mode)
return;
+ /* Data must be at or below the Cumulative TSN ACK Point to
+ * start partial delivery.
+ */
+ skb = skb_peek(&asoc->ulpq.reasm);
+ if (skb != NULL) {
+ ctsn = sctp_skb2event(skb)->tsn;
+ if (!TSN_lte(ctsn, sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)))
+ return;
+ }
+
/* If the user enabled fragment interleave socket option,
* multiple associations can enter partial delivery.
* Otherwise, we can only enter partial delivery if the
@@ -1077,12 +1105,16 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
}
/* If able to free enough room, accept this chunk. */
if (chunk && (freed >= needed)) {
- __u32 tsn;
- tsn = ntohl(chunk->subh.data_hdr->tsn);
- sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
- sctp_ulpq_tail_data(ulpq, chunk, gfp);
-
- sctp_ulpq_partial_delivery(ulpq, gfp);
+ int retval;
+ retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
+ /*
+ * Enter partial delivery if chunk has not been
+ * delivered; otherwise, drain the reassembly queue.
+ */
+ if (retval <= 0)
+ sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+ else if (retval == 1)
+ sctp_ulpq_reasm_drain(ulpq);
}
sk_mem_reclaim(asoc->base.sk);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-26 14:37 UTC (permalink / raw)
To: Lai Jiangshan
Cc: linux-doc, peterz, fweisbec, linux-kernel, walken, mingo,
linux-arch, linux, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, rostedt, rjw, namhyung, tglx, linux-arm-kernel,
netdev, oleg, vincent.guittot, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <CACvQF529c72xLuosD44CPd_y_0zJBc-tjgNQpBiD0UhbzS8XDg@mail.gmail.com>
On 02/26/2013 07:47 PM, Lai Jiangshan wrote:
> On Mon, Feb 18, 2013 at 8:38 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
>> lock-ordering related problems (unlike per-cpu locks). However, global
>> rwlocks lead to unnecessary cache-line bouncing even when there are no
>> writers present, which can slow down the system needlessly.
>
> per-CPU rwlocks(yours and mine) are the exactly same as rwlock_t in
> the view of lock dependency(except reader-C.S. can be nested in
> writer-C.S.)
> so they can deadlock in this order:
>
> spin_lock(some_lock); percpu_write_lock_irqsave()
> case CPU_DYING
> percpu_read_lock_irqsafe(); <---deadlock---> spin_lock(some_lock);
>
Yes, of course! But this is the most straight-forward of cases to fix,
because there are a well-defined number of CPU_DYING notifiers, and the fix
is to make the lock ordering same at both sides.
The real challenge is with cases like below:
spin_lock(some_lock) percpu_read_lock_irqsafe()
percpu_read_lock_irqsafe() spin_lock(some_lock)
The locking primitives (percpu_read_lock/unlock_irqsafe) have been explicitly
designed to keep cases like the above deadlock-free. Because, they ease
conversions from preempt_disable() to the new locking primitive without
lock-ordering headache.
> The lockdep can find out such dependency, but we must try our best to
> find out them before merge the patchset to mainline. We can review
> all the code of cpu_disable() and CPU_DYING and fix this kinds of lock
> dependency, but it is not easy thing, it may be a long term project.
>
:-)
That's exactly what I have done in this patchset, and that's why there
are 46 patches in this series! :-) I have run this patchset with lockdep
turned on, and verified that there are no locking problems due to the
conversion. I even posted out performance numbers from this patchset
(ie., in comparison to stop_machine), if you are curious...
http://article.gmane.org/gmane.linux.kernel/1435249
But yes, I'll have to re-verify this because of the new code that went in
during this merge window.
> ======
> And if there is any CPU_DYING code takes no locks and do some
> works(because they know they are called via stop_machine()) we need to
> add that locking code back if there is such code.(I don't know whether
> such code exist or not)
>
Yes, I explicitly verified this too. (I had mentioned this in the cover letter).
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH] drivers: net: ethernet: cpsw: consider number of slaves in interation
From: Mugunthan V N @ 2013-02-26 14:55 UTC (permalink / raw)
To: Daniel Mack; +Cc: netdev, David S. Miller
In-Reply-To: <1361887580-1770-1-git-send-email-zonque@gmail.com>
On 2/26/2013 7:36 PM, Daniel Mack wrote:
> Make cpsw_add_default_vlan() look at the actual number of slaves for its
> iteration, so boards with less than 2 slaves don't ooops at boot.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: David S. Miller <davem@davemloft.net>
Looks ok for me
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
^ permalink raw reply
* netif_receive_skb return value in bridging scenario
From: Bercaru Cristian-B43982 @ 2013-02-26 14:57 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hello!
I noticed that the return value of __netif_receive_skb is initialized to NET_RX_DROP.
If the bridging code handles the packet and successfully returns RX_HANDLER_CONSUMED, __netif_receive_skb returns the default value, NET_RX_DROP. I don't think it is fair to consider the packet dropped, since it is received successfully.
I thought the return value of __netif_receive_skb was used by Ethernet NIC drivers for updating their RX_DROP and RX_OX counters, but I studied the source code of various drivers and they seem to ignore whatever the return value. It seems strange. Then shouldn't the function header look like " void netif_receive_skb(... " ?
Thank you,
Cristian Bercaru
^ permalink raw reply
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-26 15:17 UTC (permalink / raw)
To: Lai Jiangshan
Cc: linux-doc, peterz, fweisbec, linux-kernel, Michel Lespinasse,
mingo, linux-arch, linux, xiaoguangrong, wangyun, paulmck, nikunj,
linux-pm, rusty, rostedt, rjw, namhyung, tglx, linux-arm-kernel,
Lai Jiangshan, netdev, oleg, vincent.guittot, sbw, tj, akpm,
linuxppc-dev
In-Reply-To: <CACvQF53gcuEfgM=L8-mPTt4W3GFMte-w6eyO0iv2CobAN68SNw@mail.gmail.com>
On 02/26/2013 07:04 PM, Lai Jiangshan wrote:
> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Hi Lai,
>>
>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>>> Hi, Srivatsa,
>>>
>>> The target of the whole patchset is nice for me.
>>
>> Cool! Thanks :-)
>>
>>> A question: How did you find out the such usages of
>>> "preempt_disable()" and convert them? did all are converted?
>>>
>>
>> Well, I scanned through the source tree for usages which implicitly
>> disabled CPU offline and converted them over.
>
> How do you scan? could you show the way you scan the source tree.
> I can follow your instructions for double checking.
>
Its nothing special. I grepped the source tree for anything dealing with
cpu_online_mask or its derivatives and also for functions/constructs that
rely on the cpumasks internally (eg: smp_call_function). Then I audited all
such call-sites and converted them (if needed) accordingly.
>> Its not limited to uses
>> of preempt_disable() alone - even spin_locks, rwlocks, local_irq_disable()
>> etc also help disable CPU offline. So I tried to dig out all such uses
>> and converted them. However, since the merge window is open, a lot of
>> new code is flowing into the tree. So I'll have to rescan the tree to
>> see if there are any more places to convert.
>
> I remember some code has such assumption:
> preempt_disable() (or something else)
> //the code assume that the cpu_online_map can't be changed.
> preempt_enable()
>
> It is very hard to find out all such kinds of assumptions and fixes them.
> (I notice your code mainly fixes code around send_xxxx())
>
The conversion can be carried out using the method I mentioned above.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH] libertas sdio: remove CMD_FUNC_INIT call
From: Daniel Drake @ 2013-02-26 15:33 UTC (permalink / raw)
To: Bing Zhao
Cc: Lubomir Rintel, Harro Haan, Dan Williams,
libertas-dev@lists.infradead.org, netdev@vger.kernel.org,
linux-wireless@vger.kernel.org, John W. Linville,
linux-kernel@vger.kernel.org
In-Reply-To: <477F20668A386D41ADCC57781B1F70430D9D7F8FF0@SC-VEXCH1.marvell.com>
On Mon, Feb 25, 2013 at 5:21 PM, Bing Zhao <bzhao@marvell.com> wrote:
> Do you have any concern for OLPC platforms with above change? If it doesn't seem to break OLPC I will send a patch to the list.
Looks fine to me. Thanks for investigating.
Daniel
^ permalink raw reply
* Fw: [Bug 54491] New: UFO (UDP fragmentation offload) does not work if the payload size specified is within the range ( (MTU-28) to (MTU-8) )
From: Stephen Hemminger @ 2013-02-26 15:46 UTC (permalink / raw)
To: netdev
Begin forwarded message:
Date: Tue, 26 Feb 2013 00:52:33 -0800
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 54491] New: UFO (UDP fragmentation offload) does not work if the payload size specified is within the range ( (MTU-28) to (MTU-8) )
https://bugzilla.kernel.org/show_bug.cgi?id=54491
Summary: UFO (UDP fragmentation offload) does not work if the
payload size specified is within the range ( (MTU-28)
to (MTU-8) )
Product: Networking
Version: 2.5
Kernel Version: 3.3.4
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: IPV4
AssignedTo: shemminger@linux-foundation.org
ReportedBy: hnk437@gmail.com
Regression: No
For IPv4/UDP if the payload size specified is within the range ( (MTU-28)
to (MTU-8) ) packet gets segmented in kernel even if the following netdev
features are set:
NETIF_F_SG
NETIF_F_GEN_CSUM
NETIF_F_UFO
NETIF_F_TSO
NETIF_F_GSO
NETIF_F_GSO_ROBUST
Analysis:
udp_sendmsg() computes packet length by adding UDP header size to payload size:
“ulen += sizeof(struct udphdr)”
Then the function calls ip_make_skb(), however IP header size is not added to
the packet length.
ip_make_skb() in turn calls __ip_append_data(). The same packet length is used
here.
__ip_append_data() compares the packet length with MTU. The packet length still
does not include IP header size (20 bytes).
In result, when payload size “is within the range ( (MTU-28) to (MTU-8) )”
standard branch is taken rather than ip_ufo_append_data(). Gso_size is not
computed, therefore IP fragmentation is triggered from ip_finish_output().
The issue was reproduced on 3.4.3 as well
--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
^ permalink raw reply
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Lai Jiangshan @ 2013-02-26 16:25 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Michel Lespinasse, linux-doc, peterz, fweisbec, linux-kernel,
namhyung, mingo, linux-arch, linux, xiaoguangrong, wangyun,
paulmck, nikunj, linux-pm, rusty, rostedt, rjw, vincent.guittot,
tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <512CC509.1050000@linux.vnet.ibm.com>
On Tue, Feb 26, 2013 at 10:22 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
>
> Hi Lai,
>
> I'm really not convinced that piggy-backing on lglocks would help
> us in any way. But still, let me try to address some of the points
> you raised...
>
> On 02/26/2013 06:29 PM, Lai Jiangshan wrote:
>> On Tue, Feb 26, 2013 at 5:02 PM, Srivatsa S. Bhat
>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>> On 02/26/2013 05:47 AM, Lai Jiangshan wrote:
>>>> On Tue, Feb 26, 2013 at 3:26 AM, Srivatsa S. Bhat
>>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>>> Hi Lai,
>>>>>
>>>>> On 02/25/2013 09:23 PM, Lai Jiangshan wrote:
>>>>>> Hi, Srivatsa,
>>>>>>
>>>>>> The target of the whole patchset is nice for me.
>>>>>
>>>>> Cool! Thanks :-)
>>>>>
>>> [...]
>>>
>>> Unfortunately, I see quite a few issues with the code above. IIUC, the
>>> writer and the reader both increment the same counters. So how will the
>>> unlock() code in the reader path know when to unlock which of the locks?
>>
>> The same as your code, the reader(which nested in write C.S.) just dec
>> the counters.
>
> And that works fine in my case because the writer and the reader update
> _two_ _different_ counters.
I can't find any magic in your code, they are the same counter.
/*
* It is desirable to allow the writer to acquire the percpu-rwlock
* for read (if necessary), without deadlocking or getting complaints
* from lockdep. To achieve that, just increment the reader_refcnt of
* this CPU - that way, any attempt by the writer to acquire the
* percpu-rwlock for read, will get treated as a case of nested percpu
* reader, which is safe, from a locking perspective.
*/
this_cpu_inc(pcpu_rwlock->rw_state->reader_refcnt);
> If both of them update the same counter, there
> will be a semantic clash - an increment of the counter can either mean that
> a new writer became active, or it can also indicate a nested reader. A decrement
> can also similarly have 2 meanings. And thus it will be difficult to decide
> the right action to take, based on the value of the counter.
>
>>
>>> (The counter-dropping-to-zero logic is not safe, since it can be updated
>>> due to different reasons). And now that I look at it again, in the absence
>>> of the writer, the reader is allowed to be recursive at the heavy cost of
>>> taking the global rwlock for read, every 2nd time you nest (because the
>>> spinlock is non-recursive).
>>
>> (I did not understand your comments of this part)
>> nested reader is considered seldom.
>
> No, nested readers can be _quite_ frequent. Because, potentially all users
> of preempt_disable() are readers - and its well-known how frequently we
> nest preempt_disable(). As a simple example, any atomic reader who calls
> smp_call_function() will become a nested reader, because smp_call_function()
> itself is a reader. So reader nesting is expected to be quite frequent.
>
>> But if N(>=2) nested readers happen,
>> the overhead is:
>> 1 spin_try_lock() + 1 read_lock() + (N-1) __this_cpu_inc()
>>
>
> In my patch, its just this_cpu_inc(). Note that these are _very_ hot paths.
> So every bit of optimization that you can add is worthwhile.
>
> And your read_lock() is a _global_ lock - thus, it can lead to a lot of
> cache-line bouncing. That's *exactly* why I have used per-cpu refcounts in
> my synchronization scheme, to avoid taking the global rwlock as much as possible.
>
> Another important point to note is that, the overhead we are talking about
> here, exists even when _not_ performing hotplug. And its the replacement to
> the super-fast preempt_disable(). So its extremely important to consciously
> minimize this overhead - else we'll end up slowing down the system significantly.
>
All I was considered is "nested reader is seldom", so I always
fallback to rwlock when nested.
If you like, I can add 6 lines of code, the overhead is
1 spin_try_lock()(fast path) + N __this_cpu_inc()
The overhead of your code is
2 smp_mb() + N __this_cpu_inc()
I don't see how much different.
>>> Also, this lg_rwlock implementation uses 3
>>> different data-structures - a per-cpu spinlock, a global rwlock and
>>> a per-cpu refcnt, and its not immediately apparent why you need those many
>>> or even those many varieties.
>>
>> data-structures is the same as yours.
>> fallback_reader_refcnt <--> reader_refcnt
>
> This has semantic problems, as noted above.
>
>> per-cpu spinlock <--> write_signal
>
> Acquire/release of (spin) lock is costlier than inc/dec of a counter, IIUC.
>
>> fallback_rwlock <---> global_rwlock
>>
>>> Also I see that this doesn't handle the
>>> case of interrupt-handlers also being readers.
>>
>> handled. nested reader will see the ref or take the fallback_rwlock
>>
Sorry, _reentrance_ read_lock() will see the ref or take the fallback_rwlock
>
> I'm not referring to simple nested readers here, but interrupt handlers who
> can act as readers. For starters, the arch_spin_trylock() is not safe when
> interrupt handlers can also run the same code, right? You'll need to save
> and restore interrupts at critical points in the code. Also, the __foo()
> variants used to read/update the counters are not interrupt-safe.
I must missed something.
Could you elaborate more why arch_spin_trylock() is not safe when
interrupt handlers can also run the same code?
Could you elaborate more why __this_cpu_op variants is not
interrupt-safe since they are always called paired.
> And,
> the unlock() code in the reader path is again going to be confused about
> what to do when interrupt handlers interrupt regular readers, due to the
> messed up refcount.
I still can't understand.
>
>>>
>>> IMHO, the per-cpu rwlock scheme that I have implemented in this patchset
>>> has a clean, understandable design and just enough data-structures/locks
>>> to achieve its goal and has several optimizations (like reducing the
>>> interrupts-disabled time etc) included - all in a very straight-forward
>>> manner. Since this is non-trivial, IMHO, starting from a clean slate is
>>> actually better than trying to retrofit the logic into some locking scheme
>>> which we actively want to avoid (and hence effectively we aren't even
>>> borrowing anything from!).
>>>
>>> To summarize, if you are just pointing out that we can implement the same
>>> logic by altering lglocks, then sure, I acknowledge the possibility.
>>> However, I don't think doing that actually makes it better; it either
>>> convolutes the logic unnecessarily, or ends up looking _very_ similar to
>>> the implementation in this patchset, from what I can see.
>>>
>
^ permalink raw reply
* Re: netif_receive_skb return value in bridging scenario
From: Eric Dumazet @ 2013-02-26 16:35 UTC (permalink / raw)
To: Bercaru Cristian-B43982; +Cc: netdev@vger.kernel.org
In-Reply-To: <675385003F19144A8CE636FC6B7BB7012F25EC@039-SN2MPN1-021.039d.mgd.msft.net>
On Tue, 2013-02-26 at 14:57 +0000, Bercaru Cristian-B43982 wrote:
> Hello!
>
> I noticed that the return value of __netif_receive_skb is initialized
> to NET_RX_DROP.
>
> If the bridging code handles the packet and successfully returns
> RX_HANDLER_CONSUMED, __netif_receive_skb returns the default value,
> NET_RX_DROP. I don't think it is fair to consider the packet dropped,
> since it is received successfully.
>
> I thought the return value of __netif_receive_skb was used by Ethernet
> NIC drivers for updating their RX_DROP and RX_OX counters, but I
> studied the source code of various drivers and they seem to ignore
> whatever the return value. It seems strange. Then shouldn't the
> function header look like " void netif_receive_skb(... " ?
It seems better to factorize whatever is needed in core network layer,
instead of adding code in all drivers to check netif_receive_skb()
return code.
Check the follwing "atomic_long_inc(&skb->dev->rx_dropped);"
in __netif_receive_skb_core()
For the case you mention, I guess we need the following patch
diff --git a/net/core/dev.c b/net/core/dev.c
index 17bc535..e98fb4d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3446,6 +3446,7 @@ ncls:
}
switch (rx_handler(&skb)) {
case RX_HANDLER_CONSUMED:
+ ret = NET_RX_SUCCESS;
goto unlock;
case RX_HANDLER_ANOTHER:
goto another_round;
^ permalink raw reply related
* [RFC PATCH] core: Add ioctls to control device unicast hw addresses
From: Vlad Yasevich @ 2013-02-26 16:59 UTC (permalink / raw)
To: netdev
There is currently a way to manage the multicast HW address list
through SIOCADDMULTI/SIOCDELMULTI, but there is no way to
consistently manage the unicast list. Some drivers provide
FDB based interface, but that depends on driver configuration
in almost every case.
This patch provides 2 new ioctls that allow on to add/delete
unicast HW address on a device.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
include/uapi/linux/sockios.h | 4 ++++
net/core/dev_ioctl.c | 22 ++++++++++++++++++++++
2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/include/uapi/linux/sockios.h b/include/uapi/linux/sockios.h
index 7997a50..442360e 100644
--- a/include/uapi/linux/sockios.h
+++ b/include/uapi/linux/sockios.h
@@ -127,6 +127,10 @@
/* hardware time stamping: parameters in linux/net_tstamp.h */
#define SIOCSHWTSTAMP 0x89b0
+/* Device address management */
+#define SIOCADDHWADDR 0x89c0 /* Unicast address list */
+#define SIOCDELHWADDR 0x8932
+
/* Device private ioctl calls */
/*
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 6cc0481..d84f6f9 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -314,6 +314,26 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
ifr->ifr_newname[IFNAMSIZ-1] = '\0';
return dev_change_name(dev, ifr->ifr_newname);
+ case SIOCADDHWADDR:
+ if (!ops->ndo_set_rx_mode ||
+ ifr->ifr_hwaddr.sa_family != AF_UNSPEC)
+ return -EINVAL;
+ if (!netif_device_present(dev))
+ return -ENODEV;
+ if (!is_unicast_ether_addr(addr))
+ return -EINVAL;
+ return dev_uc_add_excl(dev, ifr->ifr_hwaddr.sa_data);
+
+ case SIOCDELHWADDR:
+ if (!ops->ndo_set_rx_mode ||
+ ifr->ifr_hwaddr.sa_family != AF_UNSPEC)
+ return -EINVAL;
+ if (!netif_device_present(dev))
+ return -ENODEV;
+ if (!is_unicast_ether_addr(addr))
+ return -EINVAL;
+ return dev_uc_del(dev, ifr->ifr_hwaddr.sa_data);
+
case SIOCSHWTSTAMP:
err = net_hwtstamp_validate(ifr);
if (err)
@@ -532,6 +552,8 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
case SIOCBRADDIF:
case SIOCBRDELIF:
case SIOCSHWTSTAMP:
+ case SIOCADDHWADDR:
+ case SIOCDELHWADDR:
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
return -EPERM;
/* fall through */
--
1.7.7.6
^ permalink raw reply related
* [PATCH BUGFIX] pkt_sched: fix little service anomalies and possible crashes of qfq+
From: Paolo valente @ 2013-02-26 17:02 UTC (permalink / raw)
To: Jamal Hadi Salim, David S. Miller, shemminger
Cc: netdev, linux-kernel, fchecconi, rizzo, Paolo Valente
In-Reply-To: <20121226.151355.2215825989495032073.davem@davemloft.net>
This is a new version of the patch, with comments reformatted as
requested, and reviewed by Fabio (one of the authors of QFQ). This
version also includes some little, non-functional code improvements
suggested by Fabio.
The patch contains five fixes that remove some little service
anomalies, plus an improvement that slightly reduces the execution
time of qfq+.
As for possible crashes, in a stress test in which a few hundreds of
classes randomly switched from backlogged to non-backlogged, and the
parameters of a few tens of randomly chosen classes were changed
repeatedly and in parallel, the problems solved by these fixes
happened to combine and cause corruption of the data structures
supporting the group bucket lists.
The portions of the code interested by each fix are small and do not
overlap with each other, so I decided to provide just one patch
(I hope that this was the right choice).
1. Fix the update of eligible-group sets
Between two invocations of make_eligible, the system virtual time may
happen to grow enough that, in its binary representation, a bit with
higher order than 31 flips. This happens especially with
TSO/GSO. Before this fix, the mask used in make_eligible was computed
as (1UL<<index_of_last_flipped_bit)-1, whose value is well defined on
a 64-bit architecture, because index_of_flipped_bit <= 63, but is in
general undefined on a 32-bit architecture if index_of_flipped_bit > 31.
The fix just replaces 1UL with 1ULL.
2. Properly cap timestamps in charge_actual_service
After decreasing the number of classes, and hence the maximum budget
budgetmax for an aggregate in service, it may happen that the service
it receives before a new aggregate is chosen for service becomes
larger than budgetmax. This may cause the aggregate to be assigned an
incorrect, too high virtual finish time. The cap introduced by this
fix solves this problem.
3. Do not allow virtual time to jump if an aggregate is in service
By definition of (the algorithm of) qfq+, the system virtual time must
be pushed up only if there is no eligible aggregate. But to decide
whether this condition holds correctly, qfq+ must also check whether
no aggregate is in service (and hence eligible).
4. Prevent the budget of an aggregate from becoming negative in
qfq_dequeue
If lmax is lowered, through qfq_change_class, for a class owning
pending packets with larger size than the new value of lmax, then even
the budget of a just selected-aggregate may happen to be lower than
the length of the next packet to serve. This fix prevents the budget
from becoming negative after the packet is dequeued.
5. Start serving a just-activated aggregate immediately if the
scheduler is empty
This avoids qfq+ to be occasionally non-work-conserving.
6. Remove useless invocation of qfq_update_eligible from qfq_deactivate_agg
There is no need to invoke qfq_update_eligible in qfq_deactivate_agg
to keep qfq+ work-conserving.
Signed-off-by: Paolo Valente <paolo.valente@unimore.it>
Reviewed-by: Fabio Checconi <fchecconi@gmail.com>
---
net/sched/sch_qfq.c | 66 +++++++++++++++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 21 deletions(-)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 6ed3765..7cfa1f8 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -299,6 +299,10 @@ static void qfq_update_agg(struct qfq_sched *q, struct qfq_aggregate *agg,
new_num_classes == q->max_agg_classes - 1) /* agg no more full */
hlist_add_head(&agg->nonfull_next, &q->nonfull_aggs);
+ /* The next assignment may let
+ * agg->initial_budget > agg->budgetmax
+ * hold, we will take it into account in charge_actual_service().
+ */
agg->budgetmax = new_num_classes * agg->lmax;
new_agg_weight = agg->class_weight * new_num_classes;
agg->inv_w = ONE_FP/new_agg_weight;
@@ -819,7 +823,7 @@ static void qfq_make_eligible(struct qfq_sched *q)
unsigned long old_vslot = q->oldV >> q->min_slot_shift;
if (vslot != old_vslot) {
- unsigned long mask = (1UL << fls(vslot ^ old_vslot)) - 1;
+ unsigned long mask = (1ULL << fls(vslot ^ old_vslot)) - 1;
qfq_move_groups(q, mask, IR, ER);
qfq_move_groups(q, mask, IB, EB);
}
@@ -990,12 +994,23 @@ static inline struct sk_buff *qfq_peek_skb(struct qfq_aggregate *agg,
/* Update F according to the actual service received by the aggregate. */
static inline void charge_actual_service(struct qfq_aggregate *agg)
{
- /* compute the service received by the aggregate */
- u32 service_received = agg->initial_budget - agg->budget;
+ /* Compute the service received by the aggregate, taking into
+ * account that, after decreasing the number of classes in
+ * agg, it may happen that
+ * agg->initial_budget - agg->budget > agg->bugdetmax
+ */
+ u32 service_received = min(agg->budgetmax,
+ agg->initial_budget - agg->budget);
agg->F = agg->S + (u64)service_received * agg->inv_w;
}
+static inline void qfq_update_agg_ts(struct qfq_sched *q,
+ struct qfq_aggregate *agg,
+ enum update_reason reason);
+
+static void qfq_schedule_agg(struct qfq_sched *q, struct qfq_aggregate *agg);
+
static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
{
struct qfq_sched *q = qdisc_priv(sch);
@@ -1023,7 +1038,7 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
in_serv_agg->initial_budget = in_serv_agg->budget =
in_serv_agg->budgetmax;
- if (!list_empty(&in_serv_agg->active))
+ if (!list_empty(&in_serv_agg->active)) {
/*
* Still active: reschedule for
* service. Possible optimization: if no other
@@ -1034,8 +1049,9 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
* handle it, we would need to maintain an
* extra num_active_aggs field.
*/
- qfq_activate_agg(q, in_serv_agg, requeue);
- else if (sch->q.qlen == 0) { /* no aggregate to serve */
+ qfq_update_agg_ts(q, in_serv_agg, requeue);
+ qfq_schedule_agg(q, in_serv_agg);
+ } else if (sch->q.qlen == 0) { /* no aggregate to serve */
q->in_serv_agg = NULL;
return NULL;
}
@@ -1054,7 +1070,15 @@ static struct sk_buff *qfq_dequeue(struct Qdisc *sch)
qdisc_bstats_update(sch, skb);
agg_dequeue(in_serv_agg, cl, len);
- in_serv_agg->budget -= len;
+ /* If lmax is lowered, through qfq_change_class, for a class
+ * owning pending packets with larger size than the new value
+ * of lmax, then the following condition may hold.
+ */
+ if (unlikely(in_serv_agg->budget < len))
+ in_serv_agg->budget = 0;
+ else
+ in_serv_agg->budget -= len;
+
q->V += (u64)len * IWSUM;
pr_debug("qfq dequeue: len %u F %lld now %lld\n",
len, (unsigned long long) in_serv_agg->F,
@@ -1219,17 +1243,11 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)
cl->deficit = agg->lmax;
list_add_tail(&cl->alist, &agg->active);
- if (list_first_entry(&agg->active, struct qfq_class, alist) != cl)
- return err; /* aggregate was not empty, nothing else to do */
+ if (list_first_entry(&agg->active, struct qfq_class, alist) != cl ||
+ q->in_serv_agg == agg)
+ return err; /* non-empty or in service, nothing else to do */
- /* recharge budget */
- agg->initial_budget = agg->budget = agg->budgetmax;
-
- qfq_update_agg_ts(q, agg, enqueue);
- if (q->in_serv_agg == NULL)
- q->in_serv_agg = agg;
- else if (agg != q->in_serv_agg)
- qfq_schedule_agg(q, agg);
+ qfq_activate_agg(q, agg, enqueue);
return err;
}
@@ -1263,7 +1281,8 @@ static void qfq_schedule_agg(struct qfq_sched *q, struct qfq_aggregate *agg)
/* group was surely ineligible, remove */
__clear_bit(grp->index, &q->bitmaps[IR]);
__clear_bit(grp->index, &q->bitmaps[IB]);
- } else if (!q->bitmaps[ER] && qfq_gt(roundedS, q->V))
+ } else if (!q->bitmaps[ER] && qfq_gt(roundedS, q->V) &&
+ q->in_serv_agg == NULL)
q->V = roundedS;
grp->S = roundedS;
@@ -1286,8 +1305,15 @@ skip_update:
static void qfq_activate_agg(struct qfq_sched *q, struct qfq_aggregate *agg,
enum update_reason reason)
{
+ agg->initial_budget = agg->budget = agg->budgetmax; /* recharge budg. */
+
qfq_update_agg_ts(q, agg, reason);
- qfq_schedule_agg(q, agg);
+ if (q->in_serv_agg == NULL) { /* no aggr. in service or scheduled */
+ q->in_serv_agg = agg; /* start serving this aggregate */
+ /* update V: to be in service, agg must be eligible */
+ q->oldV = q->V = agg->S;
+ } else if (agg != q->in_serv_agg)
+ qfq_schedule_agg(q, agg);
}
static void qfq_slot_remove(struct qfq_sched *q, struct qfq_group *grp,
@@ -1359,8 +1385,6 @@ static void qfq_deactivate_agg(struct qfq_sched *q, struct qfq_aggregate *agg)
__set_bit(grp->index, &q->bitmaps[s]);
}
}
-
- qfq_update_eligible(q);
}
static void qfq_qlen_notify(struct Qdisc *sch, unsigned long arg)
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] proc connector: reject unprivileged listener bumps
From: Matt Helsley @ 2013-02-26 17:24 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, Evgeniy Polyakov, netdev, Matt Helsley
In-Reply-To: <20130226073225.GA15489@www.outflux.net>
On Mon, Feb 25, 2013 at 11:32:25PM -0800, Kees Cook wrote:
> While PROC_CN_MCAST_LISTEN/IGNORE is entirely advisory, it was possible
> for an unprivileged user to turn off notifications for all listeners by
> sending PROC_CN_MCAST_IGNORE. Instead, require the same privileges as
> required for a multicast bind.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: Evgeniy Polyakov <zbr@ioremap.net>
> Cc: Matt Helsley <matthltc@us.ibm.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/connector/cn_proc.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index fce2000..1110478 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -313,6 +313,12 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
> (task_active_pid_ns(current) != &init_pid_ns))
> return;
>
> + /* Can only change if privileged. */
I think this comment doesn't tell us anything above and beyond the
what capability check already tells us. I think you could improve it
by noting that since the connector is multicast and not namespace-safe
it's not yet appropriate to use ns_capable() here.
Acked-by: Matt Helsley <matthltc@us.ibm.com>
> + if (!capable(CAP_NET_ADMIN)) {
> + err = EPERM;
> + goto out;
> + }
^ permalink raw reply
* Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses
From: David Miller @ 2013-02-26 17:43 UTC (permalink / raw)
To: vyasevic; +Cc: netdev
In-Reply-To: <1361897970-1728-1-git-send-email-vyasevic@redhat.com>
No new ioctls please, extend the netlink interfaces as necessary
instead.
Thanks.
^ 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