* Re: [PATCH 1/1] VSOCK: Introduce VM Sockets
From: Andy King @ 2013-02-18 17:09 UTC (permalink / raw)
To: Sasha Levin
Cc: netdev, linux-kernel, virtualization, gregkh, davem, pv-drivers,
kraxel
In-Reply-To: <CA+1xoqfWGUuFjruFk1UV1BAXwzZvX4_i0MShQEpsq69B6P9ieg@mail.gmail.com>
Hi Sasha,
> > + * Specifically, we initialize the vsock_bind_table array to a size of
> > + * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
...
> Why isn't it using the kernel's linux/hashtable.h?
Gah, that's a leftover from when it was platform independent :/ I have a
patch lying around to address this, once we have some more fixes batched
up I'll send it out. Sorry about that!
Thanks!
- Andy
^ permalink raw reply
* Re: [PATCH 1/1] VSOCK: Introduce VM Sockets
From: Andy King @ 2013-02-18 17:07 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: pv-drivers, netdev, linux-kernel, virtualization, gregkh, davem
In-Reply-To: <511CC511.7080902@redhat.com>
Hi Gerd,
> > + written = transport->stream_enqueue(
> > + vsk, msg->msg_iov,
> > + len - total_written);
>
> Hmm, shouldn't we pass total_written to stream_enqueue here?
>
> In case a blocking send(big-buffer) call gets splitted into multiple
> stream_enqueue calls the second (and further) stream_enqueue calls need
> to know at which msg offset they should continue sending the data, no?
On the client side, the iov tracks it internally; see memcpy_fromiovec().
On the socket side, VMCI uses a ring buffer, so it also knows its
position internally. Or did I misunderstand the question?
Thanks!
- Andy
^ permalink raw reply
* Re: [PATCH] b43: Increase number of RX DMA slots
From: Larry Finger @ 2013-02-18 17:04 UTC (permalink / raw)
To: Bastian Bittorf
Cc: Rafał Miłecki, linville, linux-wireless, netdev, Stable
In-Reply-To: <20130218165510.GC8730@medion.lan>
On 02/18/2013 10:55 AM, Bastian Bittorf wrote:
> * Rafał Miłecki <zajec5@gmail.com> [18.02.2013 17:54]:
>> 2013/2/18 Larry Finger <Larry.Finger@lwfinger.net>:
>>
>> So probably ideal solution is to use 128 *and* fix the driver's
>> failing on overflow ;)
>
> i will run a test tomorrow an report, keep calm.
Do you have debugging turned on for b43? If so, the slot usage line at module
unload would be useful.
Larry
^ permalink raw reply
* Re: [PATCH] bonding: Fix initialize after use for 3ad machine state spinlock
From: David Miller @ 2013-02-18 17:02 UTC (permalink / raw)
To: nikolay; +Cc: netdev, andy, fubar
In-Reply-To: <1361191541-7654-1-git-send-email-nikolay@redhat.com>
When submitting multiple changes to the same area in a group, you must
number your changes.
Using tools such as "git send-email ..." will do this automatically
for you, otherwise you must put numberings into your subject lines
by hand.
^ permalink raw reply
* Re: [PATCH net-next 1/2] ip_gre: allow CSUM capable devices to handle packets
From: David Miller @ 2013-02-18 17:01 UTC (permalink / raw)
To: y; +Cc: netdev, dmitry
In-Reply-To: <1361193320-11181-1-git-send-email-y>
Your emails arrive as being from "y@broadcom.com", which doesn't
seem right at all.
Please correct this and resubmit your patches, thanks.
^ permalink raw reply
* Re: [Patch net-next v5] netpoll: fix some sparse warnings in various places
From: Jiri Pirko @ 2013-02-18 17:01 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, Eric Dumazet, Vlad Yasevich, Stephen Hemminger,
David S. Miller
In-Reply-To: <1361157749-9367-1-git-send-email-amwang@redhat.com>
Mon, Feb 18, 2013 at 04:22:29AM CET, amwang@redhat.com wrote:
>From: Cong Wang <amwang@redhat.com>
>
>This fixes sparse warnings like the one below:
>
>drivers/net/team/team.c:953:25: warning: incorrect type in return expression (different address spaces)
>drivers/net/team/team.c:953:25: expected struct netpoll_info *
>drivers/net/team/team.c:953:25: got struct netpoll_info [noderef] <asn:4>*npinfo
>
>and make these functions return bool as suggested by Jiri.
>
>Cc: Eric Dumazet <eric.dumazet@gmail.com>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Cc: Vlad Yasevich <vyasevic@redhat.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>
>Cc: David S. Miller <davem@davemloft.net>
>Signed-off-by: Cong Wang <amwang@redhat.com>
>
>---
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 94c1534..2263bf2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1300,9 +1300,9 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, g
> return err;
> }
>
>-static struct netpoll_info *bond_netpoll_info(struct bonding *bond)
>+static bool bond_netpoll_info(struct bonding *bond)
> {
>- return bond->dev->npinfo;
>+ return rcu_access_pointer(bond->dev->npinfo) != NULL;
^^^^^^^^
this is not necessary
(sorry for nitpick)
^ permalink raw reply
* Re: [PATCH 1/1] VSOCK: Introduce VM Sockets
From: Andy King @ 2013-02-18 16:56 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: pv-drivers, netdev, linux-kernel, virtualization, gregkh, davem
In-Reply-To: <511B8AAF.2060202@redhat.com>
> > + if (protocol)
> > + return -EPROTONOSUPPORT;
> > +
>
> IMO protocol == PF_VSOCK should not get rejected here.
Good catch, I've sent out a patch for this.
Thanks!
- Andy
^ permalink raw reply
* Re: [PATCH] b43: Increase number of RX DMA slots
From: Bastian Bittorf @ 2013-02-18 16:55 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Larry Finger, linville-2XuSBdqkA4R54TAoqtyWWQ,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, Bastian Bittorf, Stable
In-Reply-To: <CACna6rwLgE+o2mcRTemLFAqzdsEoA9Xyx8GyEQTh5qbv1iZ04A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [18.02.2013 17:54]:
> 2013/2/18 Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>:
>
> So probably ideal solution is to use 128 *and* fix the driver's
> failing on overflow ;)
i will run a test tomorrow an report, keep calm.
bye, bastian
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 00/45] CPU hotplug: stop_machine()-free CPU hotplug
From: Vincent Guittot @ 2013-02-18 16:50 UTC (permalink / raw)
To: Steven Rostedt
Cc: Srivatsa S. Bhat, paulmck, Russell King - ARM Linux, linux-doc,
peterz, fweisbec, linux-kernel, walken, mingo, linux-arch,
xiaoguangrong, wangyun, nikunj, linux-pm, Rusty Russell, rjw,
namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm,
linuxppc-dev
In-Reply-To: <1361201422.23152.154.camel@gandalf.local.home>
On 18 February 2013 16:30, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2013-02-18 at 11:58 +0100, Vincent Guittot wrote:
>
>> My tests have been done without cpuidle because i have some issues
>> with function tracer and cpuidle
>>
>> But the cpu hotplug and cpuidle work well when I run the tests without
>> enabling the function tracer
>>
>
> I know suspend and resume has issues with function tracing (because it
> makes things like calling smp_processor_id() crash the system), but I'm
> unaware of issues with hotplug itself. Could be some of the same issues.
>
> Can you give me more details, I'll try to investigate it.
yes for sure.
The problem is more linked to cpuidle and function tracer.
cpu hotplug and function tracer work when cpuidle is disable.
cpu hotplug and cpuidle works if i don't enable function tracer.
my platform is dead as soon as I enable function tracer if cpuidle is
enabled. It looks like some notrace are missing in my platform driver
but we haven't completely fix the issue yet
Vincent
>
> Thanks,
>
> -- Steve
>
>
^ 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-18 16:46 UTC (permalink / raw)
To: Steven Rostedt
Cc: Michel Lespinasse, tglx, peterz, tj, oleg, paulmck, rusty, mingo,
akpm, namhyung, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel, vincent.guittot
In-Reply-To: <1361205087.23152.159.camel@gandalf.local.home>
On 02/18/2013 10:01 PM, Steven Rostedt wrote:
> On Mon, 2013-02-18 at 21:51 +0530, Srivatsa S. Bhat wrote:
>> Hi Michel,
>
>> Yes.. I don't think we can avoid that. Moreover, since we _want_ unfair
>> reader/writer semantics to allow flexible locking rules and guarantee
>> deadlock-safety, having a recursive reader side is not even an issue, IMHO.
>
> Recursive unfair reader lock may guarantee deadlock-safety, but
> remember, it adds a higher probability of live-locking the write_lock.
> Which is another argument to keep this separate to cpu hotplug only.
>
True.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v6 08/46] CPU hotplug: Provide APIs to prevent CPU offline from atomic context
From: Srivatsa S. Bhat @ 2013-02-18 16:43 UTC (permalink / raw)
To: Michel Lespinasse
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, vincent.guittot
In-Reply-To: <CANN689HhVxDv+3Bn9UAxOMzj0egTvVp_c7oAmQ2nnQ8Xjn_aMA@mail.gmail.com>
On 02/18/2013 09:53 PM, Michel Lespinasse wrote:
> On Mon, Feb 18, 2013 at 8:39 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Some important design requirements and considerations:
>> -----------------------------------------------------
[...]
>> +/*
>> + * Invoked by atomic hotplug reader (a task which wants to prevent
>> + * CPU offline, but which can't afford to sleep), to prevent CPUs from
>> + * going offline. So, you can call this function from atomic contexts
>> + * (including interrupt handlers).
>> + *
>> + * Note: This does NOT prevent CPUs from coming online! It only prevents
>> + * CPUs from going offline.
>> + *
>> + * You can call this function recursively.
>> + *
>> + * Returns with preemption disabled (but interrupts remain as they are;
>> + * they are not disabled).
>> + */
>> +void get_online_cpus_atomic(void)
>> +{
>> + percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock);
>> +}
>> +EXPORT_SYMBOL_GPL(get_online_cpus_atomic);
>> +
>> +void put_online_cpus_atomic(void)
>> +{
>> + percpu_read_unlock_irqsafe(&hotplug_pcpu_rwlock);
>> +}
>> +EXPORT_SYMBOL_GPL(put_online_cpus_atomic);
>
> So, you made it clear why you want a recursive read side here.
>
> I am wondering though, if you could take care of recursive uses in
> get/put_online_cpus_atomic() instead of doing it as a property of your
> rwlock:
>
> get_online_cpus_atomic()
> {
> unsigned long flags;
> local_irq_save(flags);
> if (this_cpu_inc_return(hotplug_recusion_count) == 1)
> percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock);
> local_irq_restore(flags);
> }
>
> Once again, the idea there is to avoid baking the reader side
> recursive properties into your rwlock itself, so that it won't be
> impossible to implement reader/writer fairness into your rwlock in the
> future (which may be not be very important for the hotplug use, but
> could be when other uses get introduced).
>
Hmm, your proposal above looks good to me, at first glance.
(Sorry, I had mistaken your earlier mails to mean that you were against
recursive reader-side, while you actually meant that you didn't like
implementing the recursive reader-side logic using the recursive property
of rwlocks).
While your idea above looks good, it might introduce more complexity
in the unlock path, since this would allow nesting of heterogeneous readers
(ie., if hotplug_recursion_count == 1, you don't know whether you need to
simply decrement the counter or unlock the rwlock as well).
But I'll give this some more thought to see if we can implement this
without making it too complex. Thank you!
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: Steven Rostedt @ 2013-02-18 16:31 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Michel Lespinasse, tglx, peterz, tj, oleg, paulmck, rusty, mingo,
akpm, namhyung, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel, vincent.guittot
In-Reply-To: <5122551E.1080703@linux.vnet.ibm.com>
On Mon, 2013-02-18 at 21:51 +0530, Srivatsa S. Bhat wrote:
> Hi Michel,
> Yes.. I don't think we can avoid that. Moreover, since we _want_ unfair
> reader/writer semantics to allow flexible locking rules and guarantee
> deadlock-safety, having a recursive reader side is not even an issue, IMHO.
Recursive unfair reader lock may guarantee deadlock-safety, but
remember, it adds a higher probability of live-locking the write_lock.
Which is another argument to keep this separate to cpu hotplug only.
-- Steve
^ permalink raw reply
* Re: [PATCH v6 07/46] percpu_rwlock: Allow writers to be readers, and add lockdep annotations
From: Srivatsa S. Bhat @ 2013-02-18 16:31 UTC (permalink / raw)
To: Michel Lespinasse
Cc: linux-doc, peterz, fweisbec, linux-kernel, 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: <CANN689Ec4g7jMapv8ChAEom+gEDQ8RDVLa07CKwJ==YO+MUUrA@mail.gmail.com>
On 02/18/2013 09:21 PM, Michel Lespinasse wrote:
> On Mon, Feb 18, 2013 at 8:39 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> @@ -200,6 +217,16 @@ void percpu_write_lock_irqsave(struct percpu_rwlock *pcpu_rwlock,
>>
>> smp_mb(); /* Complete the wait-for-readers, before taking the lock */
>> write_lock_irqsave(&pcpu_rwlock->global_rwlock, *flags);
>> +
>> + /*
>> + * 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);
>
> I find this quite disgusting, but once again this may be because I
> don't like unfair recursive rwlocks.
>
:-)
> In my opinion, the alternative of explicitly not taking the read lock
> when one already has the write lock sounds *much* nicer.
I don't seem to recall any strong reasons to do it this way, so I don't have
any strong opinions on doing it this way. But one of the things to note is that,
in the CPU Hotplug case, the readers are *way* more hotter than the writer.
So avoiding extra checks/'if' conditions/memory barriers in the reader-side
is very welcome. (If we slow down the read-side, we get a performance hit
even when *not* doing hotplug!). Considering this, the logic used in this
patchset seems better, IMHO.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v6 08/46] CPU hotplug: Provide APIs to prevent CPU offline from atomic context
From: Michel Lespinasse @ 2013-02-18 16:23 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, vincent.guittot
In-Reply-To: <20130218123920.26245.56709.stgit@srivatsabhat.in.ibm.com>
On Mon, Feb 18, 2013 at 8:39 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Some important design requirements and considerations:
> -----------------------------------------------------
>
> 1. Scalable synchronization at the reader-side, especially in the fast-path
>
> Any synchronization at the atomic hotplug readers side must be highly
> scalable - avoid global single-holder locks/counters etc. Because, these
> paths currently use the extremely fast preempt_disable(); our replacement
> to preempt_disable() should not become ridiculously costly and also should
> not serialize the readers among themselves needlessly.
>
> At a minimum, the new APIs must be extremely fast at the reader side
> atleast in the fast-path, when no CPU offline writers are active.
>
> 2. preempt_disable() was recursive. The replacement should also be recursive.
>
> 3. No (new) lock-ordering restrictions
>
> preempt_disable() was super-flexible. It didn't impose any ordering
> restrictions or rules for nesting. Our replacement should also be equally
> flexible and usable.
>
> 4. No deadlock possibilities
>
> Regular per-cpu locking is not the way to go if we want to have relaxed
> rules for lock-ordering. Because, we can end up in circular-locking
> dependencies as explained in https://lkml.org/lkml/2012/12/6/290
>
> So, avoid the usual per-cpu locking schemes (per-cpu locks/per-cpu atomic
> counters with spin-on-contention etc) as much as possible, to avoid
> numerous deadlock possibilities from creeping in.
>
>
> Implementation of the design:
> ----------------------------
>
> We use per-CPU reader-writer locks for synchronization because:
>
> a. They are quite fast and scalable in the fast-path (when no writers are
> active), since they use fast per-cpu counters in those paths.
>
> b. They are recursive at the reader side.
>
> c. They provide a good amount of safety against deadlocks; they don't
> spring new deadlock possibilities on us from out of nowhere. As a
> result, they have relaxed locking rules and are quite flexible, and
> thus are best suited for replacing usages of preempt_disable() or
> local_irq_disable() at the reader side.
>
> Together, these satisfy all the requirements mentioned above.
Thanks for this detailed design explanation.
> +/*
> + * Invoked by atomic hotplug reader (a task which wants to prevent
> + * CPU offline, but which can't afford to sleep), to prevent CPUs from
> + * going offline. So, you can call this function from atomic contexts
> + * (including interrupt handlers).
> + *
> + * Note: This does NOT prevent CPUs from coming online! It only prevents
> + * CPUs from going offline.
> + *
> + * You can call this function recursively.
> + *
> + * Returns with preemption disabled (but interrupts remain as they are;
> + * they are not disabled).
> + */
> +void get_online_cpus_atomic(void)
> +{
> + percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock);
> +}
> +EXPORT_SYMBOL_GPL(get_online_cpus_atomic);
> +
> +void put_online_cpus_atomic(void)
> +{
> + percpu_read_unlock_irqsafe(&hotplug_pcpu_rwlock);
> +}
> +EXPORT_SYMBOL_GPL(put_online_cpus_atomic);
So, you made it clear why you want a recursive read side here.
I am wondering though, if you could take care of recursive uses in
get/put_online_cpus_atomic() instead of doing it as a property of your
rwlock:
get_online_cpus_atomic()
{
unsigned long flags;
local_irq_save(flags);
if (this_cpu_inc_return(hotplug_recusion_count) == 1)
percpu_read_lock_irqsafe(&hotplug_pcpu_rwlock);
local_irq_restore(flags);
}
Once again, the idea there is to avoid baking the reader side
recursive properties into your rwlock itself, so that it won't be
impossible to implement reader/writer fairness into your rwlock in the
future (which may be not be very important for the hotplug use, but
could be when other uses get introduced).
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
^ 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-18 16:21 UTC (permalink / raw)
To: Michel Lespinasse
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, vincent.guittot
In-Reply-To: <CANN689F9S7c1M8+cEpz3tsxGF34+NTRBLvxgPUOtbvav5u+RRA@mail.gmail.com>
Hi Michel,
On 02/18/2013 09:15 PM, Michel Lespinasse wrote:
> Hi Srivasta,
>
> I admit not having followed in detail the threads about the previous
> iteration, so some of my comments may have been discussed already
> before - apologies if that is the case.
>
> On Mon, Feb 18, 2013 at 8:38 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> 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 like the general idea of switching between per-cpu and global
> rwlocks as needed; however I dislike unfair locks, and especially
> unfair recursive rwlocks.
>
> If you look at rwlock_t, the main reason we haven't been able to
> implement reader/writer fairness there is because tasklist_lock makes
> use of the recursive nature of the rwlock_t read side. I'm worried
> about introducing more lock usages that would make use of the same
> property for your proposed lock.
>
> I am fine with your proposal not implementing reader/writer fairness
> from day 1, but I am worried about your proposal having a recursive
> reader side. Or, to put it another way: if your proposal didn't have a
> recursive reader side, and rwlock_t could somehow be changed to
> implement reader/writer fairness, then this property could
> automatically propagate into your proposed rwlock; but if anyone makes
> use of the recursive nature of your proposal then implementing
> reader/writer fairness later won't be as easy.
>
Actually, we don't want reader/writer fairness in this particular case.
We want deadlock safety - and in this particular case, this is guaranteed
by the unfair nature of rwlocks today.
I understand that you want to make rwlocks fair. So, I am thinking of
going ahead with Tejun's proposal - implementing our own unfair locking
scheme inside percpu-rwlocks using atomic ops or something like that, and
being completely independent of rwlock_t. That way, you can easily go
ahead with making rwlocks fair without fear of breaking CPU hotplug.
However I would much prefer making that change to percpu-rwlocks as a
separate patchset, after this patchset goes in, so that we can also see
how well this unfair logic performs in practice.
And regarding recursive reader side,... the way I see it, having a
recursive reader side is a primary requirement in this case. The reason is
that the existing reader side (with stop_machine) uses preempt_disable(),
which is recursive. So our replacement also has to be recursive.
> I see that the very next change in this series is talking about
> acquiring the read side from interrupts, so it does look like you're
> planning to make use of the recursive nature of the read side.
Yes.. I don't think we can avoid that. Moreover, since we _want_ unfair
reader/writer semantics to allow flexible locking rules and guarantee
deadlock-safety, having a recursive reader side is not even an issue, IMHO.
> I kinda
> wish you didn't, as this is exactly replicating the design of
> tasklist_lock which is IMO problematic. Your prior proposal of
> disabling interrupts during the read side had other disadvantages, but
> I think it was nice that it didn't rely on having a recursive read
> side.
>
We can have readers from non-interrupt contexts too, which depend on the
recursive property...
>> +#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))
>
> I'm personally not a fan of such one-line shorthand functions - I
> think they tend to make the code harder to read instead of easier, as
> one constantly has to refer to them to understand what's actually
> going on.
>
I got rid of most of the helper functions in this version. But I would rather
prefer retaining the above ones, because they are unwieldy and long. And IMHO
the short-hand names are pretty descriptive, so you might not actually need
to keep referring to their implementations all the time.
>> 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;
>
> I don't see anything preventing a race with the corresponding code in
> percpu_write_unlock() that sets writer_signal back to false. Did I
> miss something here ? It seems to me we don't have any guarantee that
> all writer signals will be set to true at the end of the loop...
>
Ah, thanks for pointing that out! IIRC Oleg had pointed this issue in the last
version, but back then, I hadn't fully understood what he meant. Your
explanation made it clear. I'll work on fixing this.
Thanks a lot for your review Michel!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH] b43: Increase number of RX DMA slots
From: Rafał Miłecki @ 2013-02-18 16:18 UTC (permalink / raw)
To: Larry Finger
Cc: linville-2XuSBdqkA4R54TAoqtyWWQ,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, Bastian Bittorf, Stable
In-Reply-To: <1361156480-32566-1-git-send-email-Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
2013/2/18 Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>:
> Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
> were due to overflow of the RX DMA ring buffer, which was created with 64
> slots. That finding reminded me that I was seeing similar crashed on a netbook,
> which also has a relatively slow processor. After increasing the number of
> slots to 128, runs on the netbook that previously failed now worked; however,
> I found that 109 slots had been used in one test. For that reason, the number
> of slots is being increased to 256.
So probably ideal solution is to use 128 *and* fix the driver's
failing on overflow ;)
Did you try it on some old device? Just for sure firmware&DMA will
handle it correctly.
--
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] bgmac: fix unaligned accesses to network headers
From: Rafał Miłecki @ 2013-02-18 16:14 UTC (permalink / raw)
To: Hauke Mehrtens; +Cc: davem, netdev
In-Reply-To: <1361025154-11612-1-git-send-email-hauke@hauke-m.de>
2013/2/16 Hauke Mehrtens <hauke@hauke-m.de>:
> Without this patch I get many unaligned access warnings per packet,
> this patches fixes them all. This should improve performance ony some
> systems like mips.
>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
> drivers/net/ethernet/broadcom/bgmac.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index d341090..f9b1bc8 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -301,8 +301,9 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
> bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
> ring->start);
> } else {
> - new_skb = netdev_alloc_skb(bgmac->net_dev, len);
> + new_skb = netdev_alloc_skb(bgmac->net_dev, len + 2);
> if (new_skb) {
> + skb_reserve(new_skb, 2);
I never understood that code in b44, glad you hit that warnings! ;)
The version with netdev_alloc_skb_ip_align looks much better (no more
magic numbers), b44 should be converted too.
Thanks for your patch!
--
Rafał
^ permalink raw reply
* [PATCH 4/4] VSOCK: Don't reject PF_VSOCK protocol
From: Andy King @ 2013-02-18 16:04 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization; +Cc: pv-drivers, Andy King, davem
In-Reply-To: <1361203453-1347-1-git-send-email-acking@vmware.com>
Allow our own family as the protocol value for socket creation.
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Andy King <acking@vmware.com>
---
net/vmw_vsock/af_vsock.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index c1b9e55..ca511c4 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1860,7 +1860,7 @@ static int vsock_create(struct net *net, struct socket *sock,
if (!sock)
return -EINVAL;
- if (protocol)
+ if (protocol && protocol != PF_VSOCK)
return -EPROTONOSUPPORT;
switch (sock->type) {
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/4] VSOCK: Remove hypervisor-only socket option
From: Andy King @ 2013-02-18 16:04 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization; +Cc: pv-drivers, Andy King, davem
In-Reply-To: <1361203453-1347-1-git-send-email-acking@vmware.com>
Remove hypervisor-only socket option.
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Dmitry Torokhov <dtor@vmware.com>
Signed-off-by: Andy King <acking@vmware.com>
---
include/uapi/linux/vm_sockets.h | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
index f7f2e99..df91301 100644
--- a/include/uapi/linux/vm_sockets.h
+++ b/include/uapi/linux/vm_sockets.h
@@ -52,14 +52,6 @@
#define SO_VM_SOCKETS_PEER_HOST_VM_ID 3
-/* Option name for socket's service label. Use as the option name in
- * setsockopt(3) or getsockopt(3) to set or get the service label for a socket.
- * The service label is a C-style NUL-terminated string. Only available for
- * hypervisor endpoints.
- */
-
-#define SO_VM_SOCKETS_SERVICE_LABEL 4
-
/* Option name for determining if a socket is trusted. Use as the option name
* in getsockopt(3) to determine if a socket is trusted. The value is a
* signed integer.
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/4] VSOCK: get rid of vsock_version.h
From: Andy King @ 2013-02-18 16:04 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization; +Cc: pv-drivers, Dmitry Torokhov, davem
In-Reply-To: <1361203453-1347-1-git-send-email-acking@vmware.com>
From: Dmitry Torokhov <dtor@vmware.com>
There isn't really a need to have a separate file for it.
Acked-by: Andy King <acking@vmware.com>
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
net/vmw_vsock/af_vsock.c | 3 +--
net/vmw_vsock/vsock_version.h | 22 ----------------------
2 files changed, 1 insertions(+), 24 deletions(-)
delete mode 100644 net/vmw_vsock/vsock_version.h
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index a8efe5a..c1b9e55 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -98,7 +98,6 @@
#include <net/sock.h>
#include "af_vsock.h"
-#include "vsock_version.h"
static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
static void vsock_sk_destruct(struct sock *sk);
@@ -2009,5 +2008,5 @@ EXPORT_SYMBOL_GPL(vsock_core_exit);
MODULE_AUTHOR("VMware, Inc.");
MODULE_DESCRIPTION("VMware Virtual Socket Family");
-MODULE_VERSION(VSOCK_DRIVER_VERSION_STRING);
+MODULE_VERSION("1.0.0.0-k");
MODULE_LICENSE("GPL v2");
diff --git a/net/vmw_vsock/vsock_version.h b/net/vmw_vsock/vsock_version.h
deleted file mode 100644
index 4df7f5e..0000000
--- a/net/vmw_vsock/vsock_version.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * VMware vSockets Driver
- *
- * Copyright (C) 2011-2012 VMware, Inc. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation version 2 and no later version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
- * more details.
- */
-
-#ifndef _VSOCK_VERSION_H_
-#define _VSOCK_VERSION_H_
-
-#define VSOCK_DRIVER_VERSION_PARTS { 1, 0, 0, 0 }
-#define VSOCK_DRIVER_VERSION_STRING "1.0.0.0-k"
-
-#endif /* _VSOCK_VERSION_H_ */
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/4] VSOCK: get rid of EXPORT_SYMTAB
From: Andy King @ 2013-02-18 16:04 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization; +Cc: pv-drivers, Dmitry Torokhov, davem
In-Reply-To: <1361203453-1347-1-git-send-email-acking@vmware.com>
From: Dmitry Torokhov <dtor@vmware.com>
This is the default behavior for a looooooong time.
Acked-by: Andy King <acking@vmware.com>
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
net/vmw_vsock/af_vsock.c | 2 --
net/vmw_vsock/vmci_transport.c | 2 --
2 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 54bb7bd..a8efe5a 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -76,8 +76,6 @@
*/
#include <linux/types.h>
-
-#define EXPORT_SYMTAB
#include <linux/bitops.h>
#include <linux/cred.h>
#include <linux/init.h>
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index e8a87cf..a70ace8 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -14,8 +14,6 @@
*/
#include <linux/types.h>
-
-#define EXPORT_SYMTAB
#include <linux/bitops.h>
#include <linux/cred.h>
#include <linux/init.h>
--
1.7.4.1
^ permalink raw reply related
* [PATCH 0/4] Minor vSockets fixes
From: Andy King @ 2013-02-18 16:04 UTC (permalink / raw)
To: netdev, linux-kernel, virtualization; +Cc: davem, pv-drivers, kraxel, Andy King
Minor vSockets fixes, two of which were reported on LKML.
Andy King (2):
VSOCK: Remove hypervisor-only socket option
VSOCK: Don't reject PF_VSOCK protocol
Dmitry Torokhov (2):
VSOCK: get rid of EXPORT_SYMTAB
VSOCK: get rid of vsock_version.h
include/uapi/linux/vm_sockets.h | 8 --------
net/vmw_vsock/af_vsock.c | 7 ++-----
net/vmw_vsock/vmci_transport.c | 2 --
net/vmw_vsock/vsock_version.h | 22 ----------------------
4 files changed, 2 insertions(+), 37 deletions(-)
delete mode 100644 net/vmw_vsock/vsock_version.h
--
1.7.4.1
^ permalink raw reply
* Re: [PATCH v6 07/46] percpu_rwlock: Allow writers to be readers, and add lockdep annotations
From: Michel Lespinasse @ 2013-02-18 15:51 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, vincent.guittot
In-Reply-To: <20130218123913.26245.7713.stgit@srivatsabhat.in.ibm.com>
On Mon, Feb 18, 2013 at 8:39 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> @@ -200,6 +217,16 @@ void percpu_write_lock_irqsave(struct percpu_rwlock *pcpu_rwlock,
>
> smp_mb(); /* Complete the wait-for-readers, before taking the lock */
> write_lock_irqsave(&pcpu_rwlock->global_rwlock, *flags);
> +
> + /*
> + * 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);
I find this quite disgusting, but once again this may be because I
don't like unfair recursive rwlocks.
In my opinion, the alternative of explicitly not taking the read lock
when one already has the write lock sounds *much* nicer.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
^ permalink raw reply
* [PATCH ipsec-next] xfrm: allow to avoid copying DSCP during encapsulation
From: Nicolas Dichtel @ 2013-02-18 15:32 UTC (permalink / raw)
To: steffen.klassert, herbert, davem; +Cc: netdev, Nicolas Dichtel
By default, DSCP is copying during encapsulation.
Copying the DSCP in IPsec tunneling may be a bit dangerous because packets with
different DSCP may get reordered relative to each other in the network and then
dropped by the remote IPsec GW if the reordering becomes too big compared to the
replay window.
It is possible to avoid this copy with netfilter rules, but it's very convenient
to be able to configure it for each SA directly.
This patch adds a toogle for this purpose. By default, it's not set to maintain
backward compatibility.
Field flags in struct xfrm_usersa_info is full, hence I add a new attribute.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
include/net/xfrm.h | 1 +
include/uapi/linux/xfrm.h | 3 +++
net/ipv4/xfrm4_mode_tunnel.c | 8 ++++++--
net/ipv6/xfrm6_mode_tunnel.c | 7 +++++--
net/xfrm/xfrm_user.c | 13 +++++++++++++
5 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 30f3e5b..c5f12da 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -162,6 +162,7 @@ struct xfrm_state {
xfrm_address_t saddr;
int header_len;
int trailer_len;
+ u32 extra_flags;
} props;
struct xfrm_lifetime_cfg lft;
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 28e493b..f07f422 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -297,6 +297,7 @@ enum xfrm_attr_type_t {
XFRMA_MARK, /* struct xfrm_mark */
XFRMA_TFCPAD, /* __u32 */
XFRMA_REPLAY_ESN_VAL, /* struct xfrm_replay_esn */
+ XFRMA_SA_EXTRA_FLAGS, /* __u32 */
__XFRMA_MAX
#define XFRMA_MAX (__XFRMA_MAX - 1)
@@ -367,6 +368,8 @@ struct xfrm_usersa_info {
#define XFRM_STATE_ESN 128
};
+#define XFRM_STATE_EXTRA_FLAGS_DONT_ENCAP_DSCP 1
+
struct xfrm_usersa_id {
xfrm_address_t daddr;
__be32 spi;
diff --git a/net/ipv4/xfrm4_mode_tunnel.c b/net/ipv4/xfrm4_mode_tunnel.c
index ddee0a0..0273555 100644
--- a/net/ipv4/xfrm4_mode_tunnel.c
+++ b/net/ipv4/xfrm4_mode_tunnel.c
@@ -103,8 +103,12 @@ static int xfrm4_mode_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
top_iph->protocol = xfrm_af2proto(skb_dst(skb)->ops->family);
- /* DS disclosed */
- top_iph->tos = INET_ECN_encapsulate(XFRM_MODE_SKB_CB(skb)->tos,
+ /* DS disclosing depends on XFRM_STATE_EXTRA_FLAGS_DONT_ENCAP_DSCP */
+ if (x->props.extra_flags & XFRM_STATE_EXTRA_FLAGS_DONT_ENCAP_DSCP)
+ top_iph->tos = 0;
+ else
+ top_iph->tos = XFRM_MODE_SKB_CB(skb)->tos;
+ top_iph->tos = INET_ECN_encapsulate(top_iph->tos,
XFRM_MODE_SKB_CB(skb)->tos);
flags = x->props.flags;
diff --git a/net/ipv6/xfrm6_mode_tunnel.c b/net/ipv6/xfrm6_mode_tunnel.c
index 9f2095b..7c1cc59 100644
--- a/net/ipv6/xfrm6_mode_tunnel.c
+++ b/net/ipv6/xfrm6_mode_tunnel.c
@@ -49,8 +49,11 @@ static int xfrm6_mode_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
sizeof(top_iph->flow_lbl));
top_iph->nexthdr = xfrm_af2proto(skb_dst(skb)->ops->family);
- dsfield = XFRM_MODE_SKB_CB(skb)->tos;
- dsfield = INET_ECN_encapsulate(dsfield, dsfield);
+ if (x->props.extra_flags & XFRM_STATE_EXTRA_FLAGS_DONT_ENCAP_DSCP)
+ dsfield = 0;
+ else
+ dsfield = XFRM_MODE_SKB_CB(skb)->tos;
+ dsfield = INET_ECN_encapsulate(dsfield, XFRM_MODE_SKB_CB(skb)->tos);
if (x->props.flags & XFRM_STATE_NOECN)
dsfield &= ~INET_ECN_MASK;
ipv6_change_dsfield(top_iph, 0, dsfield);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index eb872b2..8cf7f5f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -515,6 +515,9 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
copy_from_user_state(x, p);
+ if (attrs[XFRMA_SA_EXTRA_FLAGS])
+ x->props.extra_flags = nla_get_u32(attrs[XFRMA_SA_EXTRA_FLAGS]);
+
if ((err = attach_aead(&x->aead, &x->props.ealgo,
attrs[XFRMA_ALG_AEAD])))
goto error;
@@ -779,6 +782,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
copy_to_user_state(x, p);
+ if (x->props.extra_flags) {
+ ret = nla_put_u32(skb, XFRMA_SA_EXTRA_FLAGS,
+ x->props.extra_flags);
+ if (ret)
+ goto out;
+ }
+
if (x->coaddr) {
ret = nla_put(skb, XFRMA_COADDR, sizeof(*x->coaddr), x->coaddr);
if (ret)
@@ -2302,6 +2312,7 @@ static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
[XFRMA_MARK] = { .len = sizeof(struct xfrm_mark) },
[XFRMA_TFCPAD] = { .type = NLA_U32 },
[XFRMA_REPLAY_ESN_VAL] = { .len = sizeof(struct xfrm_replay_state_esn) },
+ [XFRMA_SA_EXTRA_FLAGS] = { .type = NLA_U32 },
};
static struct xfrm_link {
@@ -2495,6 +2506,8 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
x->security->ctx_len);
if (x->coaddr)
l += nla_total_size(sizeof(*x->coaddr));
+ if (x->props.extra_flags)
+ l += nla_total_size(sizeof(x->props.extra_flags));
/* Must count x->lastused as it may become non-zero behind our back. */
l += nla_total_size(sizeof(u64));
--
1.8.0.1
^ permalink raw reply related
* Re: [PATCH v6 04/46] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Michel Lespinasse @ 2013-02-18 15:45 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, vincent.guittot
In-Reply-To: <20130218123856.26245.46705.stgit@srivatsabhat.in.ibm.com>
Hi Srivasta,
I admit not having followed in detail the threads about the previous
iteration, so some of my comments may have been discussed already
before - apologies if that is the case.
On Mon, Feb 18, 2013 at 8:38 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 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 like the general idea of switching between per-cpu and global
rwlocks as needed; however I dislike unfair locks, and especially
unfair recursive rwlocks.
If you look at rwlock_t, the main reason we haven't been able to
implement reader/writer fairness there is because tasklist_lock makes
use of the recursive nature of the rwlock_t read side. I'm worried
about introducing more lock usages that would make use of the same
property for your proposed lock.
I am fine with your proposal not implementing reader/writer fairness
from day 1, but I am worried about your proposal having a recursive
reader side. Or, to put it another way: if your proposal didn't have a
recursive reader side, and rwlock_t could somehow be changed to
implement reader/writer fairness, then this property could
automatically propagate into your proposed rwlock; but if anyone makes
use of the recursive nature of your proposal then implementing
reader/writer fairness later won't be as easy.
I see that the very next change in this series is talking about
acquiring the read side from interrupts, so it does look like you're
planning to make use of the recursive nature of the read side. I kinda
wish you didn't, as this is exactly replicating the design of
tasklist_lock which is IMO problematic. Your prior proposal of
disabling interrupts during the read side had other disadvantages, but
I think it was nice that it didn't rely on having a recursive read
side.
> +#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))
I'm personally not a fan of such one-line shorthand functions - I
think they tend to make the code harder to read instead of easier, as
one constantly has to refer to them to understand what's actually
going on.
> 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;
I don't see anything preventing a race with the corresponding code in
percpu_write_unlock() that sets writer_signal back to false. Did I
miss something here ? It seems to me we don't have any guarantee that
all writer signals will be set to true at the end of the loop...
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
^ 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