On Fri, Feb 10, 2012 at 02:53:20PM -0800, Vasu Dev wrote:
> On Thu, 2012-02-09 at 11:34 -0500, Neil Horman wrote:
> > commit 859b7b649ab58ee5cbfb761491317d5b315c1b0f introduced the ability to
> > call
> > fcoe_recv_frame in softirq context. While this is beneficial to
> > performance,
> > its not safe to do, as it breaks the serialization of access to the lport
> > structure (i.e. when an fcoe interface is being torn down, theres no way to
> > serialize the teardown effort with the completion of receieve operations
> > occuring in softirq context. As a result, lport (and other) data
> > structures can
> > be read and modified in parallel leading to corruption. Most notable is the
> > vport list, which is protected by a mutex, that will cause a panic if a
> > softirq
> > receive while said mutex is locked. Additionaly, the ema_list, discussed
> > here:
>
> >
> > http://lists.open-fcoe.org/pipermail/devel/2012-February/011947.html
> >
> > Can be corrupted if a list traversal occurs in softirq context at the same
> > time
> > as a list delete in process context. And generally the lport state
> > variables
> > will not be stable, and may lead to unpredictable results.
> >
> > The most direct fix is to remove the bits from the above commit that allowed
> > fcoe_recv_frame to be called in softirq context. We just force all frames
> > to be
> > handled by the per-cpu rx threads. This will allow the fcoe_if_destroy's
> > use of
> > fcoe_percpu_clean to function properly, ensuring that no frames are being
> > received while the lport is being torn down.
>
> This approach looks fine as we discussed before, it might be worth
> checking to restore soft irq later by checking perf if we could see
> possible fix even with added rcu locking is fine as rcu are cheap but at
> the moment don't know how. May think of fixing by removing too many per
> cpu fcoe rx thread and instead using either work queues or soft irq
> context for rx path only.
>
Agreed, my thought was to use rcu locking to toggle between two list heads in rx
thread context. i.e. for each rx thread :
1) lock the rx_list lock
2) do an rcu_assign_pointer of one of two list heads to a list head pointer
3) call synchronize_rcu
4) process all items on the list that was not assigned to the list pointer
If we do that, then the softirq context can just take the rcu_read_lock,
dereference the list head pointer in (2), do a list_add, and a rcu_read_unlock.
That would be considerably more lightweight than sharing the lock between
softirq and process contexts. perf runs could give us a more exact speedup.
I've got some FCoE backporting to do next week, I'll look into this as soon as
thats done.
> The patch covers the case of frames already queued to rx thread but if
> any soft irq blocked with bh disabled then those frames won't get caught
> here by fcoe_percpu_clean. I'm sending patch to cover that case as well
> along with fixes for some more issues found during this discussion as
> RFC first and then will get them out once tested.
>
Yeah, I think you're right. I would imagine that we could fix that by modifying
for destructor on the skb that posts to the completion structure in
foce_percpu_clean, such that it also marks the lport as being dead, so that
subsequently queued frames bound for that lport are immediately discarded. But
you clearly have something in mind, I'll take a look at what you post.
> Reviewed-by: Vasu Dev <vasu....@intel.com
Thanks!
Neil
>
> >
> > Signed-off-by: Neil Horman <nhor...@tuxdriver.com
> > CC: kiran.pa...@intel.com
> > CC: bprak...@broadcom.com
> > CC: robert.w.l...@intel.com
> > CC: Chris Leech <christopher.le...@intel.com>
> > CC: John Fastabend <john.r.fastab...@intel.com>
> > CC: Vasu Dev <vasu....@linux.intel.com>
> > ---
> > drivers/scsi/fcoe/fcoe.c | 27 ++++++++++-----------------
> > 1 files changed, 10 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index 0233242..2a377f9 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1455,24 +1455,17 @@ static int fcoe_rcv(struct sk_buff *skb, struct
> > net_device *netdev,
> > * so we're free to queue skbs into it's queue.
> > */
> >
> > - /* If this is a SCSI-FCP frame, and this is already executing on the
> > - * correct CPU, and the queue for this CPU is empty, then go ahead
> > - * and process the frame directly in the softirq context.
> > - * This lets us process completions without context switching from the
> > - * NET_RX softirq, to our receive processing thread, and then back to
> > - * BLOCK softirq context.
> > + /*
> > + * Note: We used to have a set of conditions under which we would
> > + * call fcoe_recv_frame directly, rather than queuing to the rx list
> > + * as it could save a few cycles, but doing so is prohibited, as
> > + * fcoe_recv_frame has several paths that may sleep, which is forbidden
> > + * in softirq context.
> > */
> > - if (fh->fh_type == FC_TYPE_FCP &&
> > - cpu == smp_processor_id() &&
> > - skb_queue_empty(&fps->fcoe_rx_list)) {
> > - spin_unlock_bh(&fps->fcoe_rx_list.lock);
> > - fcoe_recv_frame(skb);
> > - } else {
> > - __skb_queue_tail(&fps->fcoe_rx_list, skb);
> > - if (fps->fcoe_rx_list.qlen == 1)
> > - wake_up_process(fps->thread);
> > - spin_unlock_bh(&fps->fcoe_rx_list.lock);
> > - }
> > + __skb_queue_tail(&fps->fcoe_rx_list, skb);
> > + if (fps->fcoe_rx_list.qlen == 1)
> > + wake_up_process(fps->thread);
> > + spin_unlock_bh(&fps->fcoe_rx_list.lock);
> >
> > return 0;
> > err:
>
>
>
_______________________________________________
devel mailing list
devel@open-fcoe.org
https://lists.open-fcoe.org/mailman/listinfo/devel
