public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
From: Robert Love <robert.w.love@intel.com>
To: linux-scsi@vger.kernel.org
Cc: Neil Horman <nhorman@tuxdriver.com>, Vasu Dev <vasu.dev@intel.com>
Subject: [PATCH 02/10] fcoe: Ensure fcoe_recv_frame is always called in process context
Date: Fri, 09 Mar 2012 14:49:48 -0800	[thread overview]
Message-ID: <20120309224948.6515.58974.stgit@localhost6.localdomain6> (raw)
In-Reply-To: <20120309224937.6515.83801.stgit@localhost6.localdomain6>

From: Neil Horman <nhorman@tuxdriver.com>

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.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reviewed-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@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 2789581..22ae295 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1463,24 +1463,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:


  parent reply	other threads:[~2012-03-09 22:50 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-09 22:49 [PATCH 00/10] libfc, libfcoe and fcoe updates for scsi-misc Robert Love
2012-03-09 22:49 ` [PATCH 01/10] fcoe: avoid getting into dev_queue_xmit with bottom halves disabled Robert Love
2012-03-09 23:50   ` Bhanu Prakash Gollapudi
2012-03-10  0:12     ` Love, Robert W
     [not found]       ` <4F5A9C53.5040200-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2012-03-12 22:51         ` Love, Robert W
     [not found]     ` <4F5A9754.6080009-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2012-03-11 17:43       ` Neil Horman
2012-03-12  3:25         ` Bhanu Prakash Gollapudi
2012-03-12 10:42           ` Neil Horman
2012-03-09 22:49 ` Robert Love [this message]
2012-03-09 22:49 ` [PATCH 03/10] libfcoe: Do not sends FDISCs before FLOGI during CVL Robert Love
2012-03-09 22:49 ` [PATCH 04/10] libfc: update fc_host mfs along with updating lport->mfs Robert Love
2012-03-09 22:50 ` [PATCH 05/10] libfcoe: Support extra MAC descriptor to be used as FCoE MAC Robert Love
2012-03-09 22:50 ` [PATCH 06/10] foce: remove bh disable from fcoe sw transport rcv function Robert Love
2012-03-09 22:50 ` [PATCH 07/10] bnx2fc: Remove bh disable in softirq context Robert Love
2012-03-09 22:50 ` [PATCH 08/10] fcoe: remove frame dropping code from fcoe_percpu_clean Robert Love
2012-03-09 22:50 ` [PATCH 09/10] fcoe: reduce contention for fcoe_rx_list lock [v2] Robert Love
2012-03-09 22:50 ` [PATCH 10/10] libfc: fcoe_transport_create fails in single-CPU environment Robert Love

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120309224948.6515.58974.stgit@localhost6.localdomain6 \
    --to=robert.w.love@intel.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=vasu.dev@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox