[PATCH] bnx2fc: Improve stats update mechanism

[PATCH] bnx2fc: Improve stats update mechanism

[Neil Horman]

Recently had this warning reported:

[  290.489047] Call Trace:
[  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
[  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
[  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
[  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
[  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
[  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
[  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
[  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
[  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
[  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
[  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80

Its due to the fact that we call a potentially sleeping function from the bnx2fc
rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
variable stats lookup in bnx2fc_l2_rcv_thread.

Easy enough fix, we can just move the stats collection later in the function
where we are sure we won't preempt or sleep.  This also allows us to not have to
enable pre-emption when doing a per-cpu lookup, since we're certain not to get
rescheduled.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: linux-scsi@vger.kernel.org
CC: Robert Love <robert.w.love@intel.com>
CC: Vasu Dev <vasu.dev@intel.com>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 9b94850..475eee5 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -516,23 +516,17 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 	skb_pull(skb, sizeof(struct fcoe_hdr));
 	fr_len = skb->len - sizeof(struct fcoe_crc_eof);
 
-	stats = per_cpu_ptr(lport->stats, get_cpu());
-	stats->RxFrames++;
-	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
-
 	fp = (struct fc_frame *)skb;
 	fc_frame_init(fp);
 	fr_dev(fp) = lport;
 	fr_sof(fp) = hp->fcoe_sof;
 	if (skb_copy_bits(skb, fr_len, &crc_eof, sizeof(crc_eof))) {
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
 	fr_eof(fp) = crc_eof.fcoe_eof;
 	fr_crc(fp) = crc_eof.fcoe_crc32;
 	if (pskb_trim(skb, fr_len)) {
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
@@ -544,7 +538,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 		port = lport_priv(vn_port);
 		if (!ether_addr_equal(port->data_src_addr, dest_mac)) {
 			BNX2FC_HBA_DBG(lport, "fpma mismatch\n");
-			put_cpu();
 			kfree_skb(skb);
 			return;
 		}
@@ -552,7 +545,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 	if (fh->fh_r_ctl == FC_RCTL_DD_SOL_DATA &&
 	    fh->fh_type == FC_TYPE_FCP) {
 		/* Drop FCP data. We dont this in L2 path */
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
@@ -562,7 +554,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 		case ELS_LOGO:
 			if (ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
 				/* drop non-FIP LOGO */
-				put_cpu();
 				kfree_skb(skb);
 				return;
 			}
@@ -572,22 +563,23 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 
 	if (fh->fh_r_ctl == FC_RCTL_BA_ABTS) {
 		/* Drop incoming ABTS */
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
 
+	stats = per_cpu_ptr(lport->stats, smp_processor_id());
+	stats->RxFrames++;
+	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
+
 	if (le32_to_cpu(fr_crc(fp)) !=
 			~crc32(~0, skb->data, fr_len)) {
 		if (stats->InvalidCRCCount < 5)
 			printk(KERN_WARNING PFX "dropping frame with "
 			       "CRC error\n");
 		stats->InvalidCRCCount++;
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
-	put_cpu();
 	fc_exch_recv(lport, fp);
 }
 
-- 
1.8.3.1

Re: [Open-FCoE] [PATCH] bnx2fc: Improve stats update mechanism

[Eddie Wai]

Thanks for fixing this.  The patch generally looks good, but I do have a
few comments.

On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
> Recently had this warning reported:
> 
> [  290.489047] Call Trace:
> [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
> [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
> [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
> [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
> [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
> [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
> [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
> [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
> [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
> [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> 
> Its due to the fact that we call a potentially sleeping function from the bnx2fc
> rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
> variable stats lookup in bnx2fc_l2_rcv_thread.
> 
> Easy enough fix, we can just move the stats collection later in the function
> where we are sure we won't preempt or sleep.  This also allows us to not have to
> enable pre-emption when doing a per-cpu lookup, since we're certain not to get
You mean this allows us to not have to 'disable' pre-emption, right? 

Can you elaborate on how we can be sure that we won't get preempted
immediately after retrieving the CPU variable?  I would think it'll be
okay to call get_cpu at this stage as there won't be any sleepable mutex
lock calls before the put_cpu.
> rescheduled.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: linux-scsi@vger.kernel.org
> CC: Robert Love <robert.w.love@intel.com>
> CC: Vasu Dev <vasu.dev@intel.com>
> ---
>  drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> index 9b94850..475eee5 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> @@ -516,23 +516,17 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>  	skb_pull(skb, sizeof(struct fcoe_hdr));
>  	fr_len = skb->len - sizeof(struct fcoe_crc_eof);
>  
> -	stats = per_cpu_ptr(lport->stats, get_cpu());
> -	stats->RxFrames++;
> -	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
> -
>  	fp = (struct fc_frame *)skb;
>  	fc_frame_init(fp);
>  	fr_dev(fp) = lport;
>  	fr_sof(fp) = hp->fcoe_sof;
>  	if (skb_copy_bits(skb, fr_len, &crc_eof, sizeof(crc_eof))) {
> -		put_cpu();
>  		kfree_skb(skb);
>  		return;
>  	}
>  	fr_eof(fp) = crc_eof.fcoe_eof;
>  	fr_crc(fp) = crc_eof.fcoe_crc32;
>  	if (pskb_trim(skb, fr_len)) {
> -		put_cpu();
>  		kfree_skb(skb);
>  		return;
>  	}
> @@ -544,7 +538,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>  		port = lport_priv(vn_port);
>  		if (!ether_addr_equal(port->data_src_addr, dest_mac)) {
>  			BNX2FC_HBA_DBG(lport, "fpma mismatch\n");
> -			put_cpu();
>  			kfree_skb(skb);
>  			return;
>  		}
> @@ -552,7 +545,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>  	if (fh->fh_r_ctl == FC_RCTL_DD_SOL_DATA &&
>  	    fh->fh_type == FC_TYPE_FCP) {
>  		/* Drop FCP data. We dont this in L2 path */
> -		put_cpu();
>  		kfree_skb(skb);
>  		return;
>  	}
> @@ -562,7 +554,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>  		case ELS_LOGO:
>  			if (ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
>  				/* drop non-FIP LOGO */
> -				put_cpu();
>  				kfree_skb(skb);
>  				return;
>  			}
> @@ -572,22 +563,23 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>  
>  	if (fh->fh_r_ctl == FC_RCTL_BA_ABTS) {
>  		/* Drop incoming ABTS */
> -		put_cpu();
>  		kfree_skb(skb);
>  		return;
>  	}
>  
> +	stats = per_cpu_ptr(lport->stats, smp_processor_id());
> +	stats->RxFrames++;
> +	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
> +
>  	if (le32_to_cpu(fr_crc(fp)) !=
>  			~crc32(~0, skb->data, fr_len)) {
>  		if (stats->InvalidCRCCount < 5)
>  			printk(KERN_WARNING PFX "dropping frame with "
>  			       "CRC error\n");
>  		stats->InvalidCRCCount++;
> -		put_cpu();
>  		kfree_skb(skb);
>  		return;
>  	}
> -	put_cpu();
>  	fc_exch_recv(lport, fp);
>  }
>  

Re: [Open-FCoE] [PATCH] bnx2fc: Improve stats update mechanism

[Neil Horman]

On Fri, May 30, 2014 at 02:59:43PM -0700, Eddie Wai wrote:
> Thanks for fixing this.  The patch generally looks good, but I do have a
> few comments.
> 
> On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
> > Recently had this warning reported:
> > 
> > [  290.489047] Call Trace:
> > [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
> > [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
> > [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
> > [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
> > [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
> > [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
> > [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
> > [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
> > [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
> > [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > 
> > Its due to the fact that we call a potentially sleeping function from the bnx2fc
> > rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
> > variable stats lookup in bnx2fc_l2_rcv_thread.
> > 
> > Easy enough fix, we can just move the stats collection later in the function
> > where we are sure we won't preempt or sleep.  This also allows us to not have to
> > enable pre-emption when doing a per-cpu lookup, since we're certain not to get
> You mean this allows us to not have to 'disable' pre-emption, right? 
> 
> Can you elaborate on how we can be sure that we won't get preempted
> immediately after retrieving the CPU variable?  I would think it'll be
> okay to call get_cpu at this stage as there won't be any sleepable mutex
> lock calls before the put_cpu.
We can't be sure, but I would assert it doesn't really matter at this point.
The area in which we update stats is so small that, even if we do hit the
unlikely chance that we get pre-empted, and then rescheduled on a different cpu,
it won't matter.  We could add the get_cpu/put_cpu back if you're really intent
on it, but I'm not sure its worthwhile given that this is a hot path.

Neil

> > rescheduled.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: linux-scsi@vger.kernel.org
> > CC: Robert Love <robert.w.love@intel.com>
> > CC: Vasu Dev <vasu.dev@intel.com>
> > ---
> >  drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> > index 9b94850..475eee5 100644
> > --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> > @@ -516,23 +516,17 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
> >  	skb_pull(skb, sizeof(struct fcoe_hdr));
> >  	fr_len = skb->len - sizeof(struct fcoe_crc_eof);
> >  
> > -	stats = per_cpu_ptr(lport->stats, get_cpu());
> > -	stats->RxFrames++;
> > -	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
> > -
> >  	fp = (struct fc_frame *)skb;
> >  	fc_frame_init(fp);
> >  	fr_dev(fp) = lport;
> >  	fr_sof(fp) = hp->fcoe_sof;
> >  	if (skb_copy_bits(skb, fr_len, &crc_eof, sizeof(crc_eof))) {
> > -		put_cpu();
> >  		kfree_skb(skb);
> >  		return;
> >  	}
> >  	fr_eof(fp) = crc_eof.fcoe_eof;
> >  	fr_crc(fp) = crc_eof.fcoe_crc32;
> >  	if (pskb_trim(skb, fr_len)) {
> > -		put_cpu();
> >  		kfree_skb(skb);
> >  		return;
> >  	}
> > @@ -544,7 +538,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
> >  		port = lport_priv(vn_port);
> >  		if (!ether_addr_equal(port->data_src_addr, dest_mac)) {
> >  			BNX2FC_HBA_DBG(lport, "fpma mismatch\n");
> > -			put_cpu();
> >  			kfree_skb(skb);
> >  			return;
> >  		}
> > @@ -552,7 +545,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
> >  	if (fh->fh_r_ctl == FC_RCTL_DD_SOL_DATA &&
> >  	    fh->fh_type == FC_TYPE_FCP) {
> >  		/* Drop FCP data. We dont this in L2 path */
> > -		put_cpu();
> >  		kfree_skb(skb);
> >  		return;
> >  	}
> > @@ -562,7 +554,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
> >  		case ELS_LOGO:
> >  			if (ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
> >  				/* drop non-FIP LOGO */
> > -				put_cpu();
> >  				kfree_skb(skb);
> >  				return;
> >  			}
> > @@ -572,22 +563,23 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
> >  
> >  	if (fh->fh_r_ctl == FC_RCTL_BA_ABTS) {
> >  		/* Drop incoming ABTS */
> > -		put_cpu();
> >  		kfree_skb(skb);
> >  		return;
> >  	}
> >  
> > +	stats = per_cpu_ptr(lport->stats, smp_processor_id());
> > +	stats->RxFrames++;
> > +	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
> > +
> >  	if (le32_to_cpu(fr_crc(fp)) !=
> >  			~crc32(~0, skb->data, fr_len)) {
> >  		if (stats->InvalidCRCCount < 5)
> >  			printk(KERN_WARNING PFX "dropping frame with "
> >  			       "CRC error\n");
> >  		stats->InvalidCRCCount++;
> > -		put_cpu();
> >  		kfree_skb(skb);
> >  		return;
> >  	}
> > -	put_cpu();
> >  	fc_exch_recv(lport, fp);
> >  }
> >  
> 
> 
> 

Re: [Open-FCoE] [PATCH] bnx2fc: Improve stats update mechanism

[Eddie Wai]

On May 30, 2014, at 7:48 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:

> On Fri, May 30, 2014 at 02:59:43PM -0700, Eddie Wai wrote:
>> Thanks for fixing this.  The patch generally looks good, but I do have a
>> few comments.
>> 
>> On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
>>> Recently had this warning reported:
>>> 
>>> [  290.489047] Call Trace:
>>> [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
>>> [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
>>> [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
>>> [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
>>> [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
>>> [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
>>> [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
>>> [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
>>> [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
>>> [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
>>> [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
>>> 
>>> Its due to the fact that we call a potentially sleeping function from the bnx2fc
>>> rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
>>> variable stats lookup in bnx2fc_l2_rcv_thread.
>>> 
>>> Easy enough fix, we can just move the stats collection later in the function
>>> where we are sure we won't preempt or sleep.  This also allows us to not have to
>>> enable pre-emption when doing a per-cpu lookup, since we're certain not to get
>> You mean this allows us to not have to 'disable' pre-emption, right? 
>> 
>> Can you elaborate on how we can be sure that we won't get preempted
>> immediately after retrieving the CPU variable?  I would think it'll be
>> okay to call get_cpu at this stage as there won't be any sleepable mutex
>> lock calls before the put_cpu.
> We can't be sure, but I would assert it doesn't really matter at this point.
> The area in which we update stats is so small that, even if we do hit the
> unlikely chance that we get pre-empted, and then rescheduled on a different cpu,
> it won't matter.  We could add the get_cpu/put_cpu back if you're really intent
> on it, but I'm not sure its worthwhile given that this is a hot path.
I agree with your assessment.  But code wise just so bnx2fc is consistent to the software FCoE counterpart, I would advice to use the same get_cpu to retrieve that stats CPU variable.  Thanks.
> 
> Neil
> 
>>> rescheduled.
>>> 
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> CC: linux-scsi@vger.kernel.org
>>> CC: Robert Love <robert.w.love@intel.com>
>>> CC: Vasu Dev <vasu.dev@intel.com>
>>> ---
>>> drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 16 ++++------------
>>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> index 9b94850..475eee5 100644
>>> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> @@ -516,23 +516,17 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>>>    skb_pull(skb, sizeof(struct fcoe_hdr));
>>>    fr_len = skb->len - sizeof(struct fcoe_crc_eof);
>>> 
>>> -    stats = per_cpu_ptr(lport->stats, get_cpu());
>>> -    stats->RxFrames++;
>>> -    stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
>>> -
>>>    fp = (struct fc_frame *)skb;
>>>    fc_frame_init(fp);
>>>    fr_dev(fp) = lport;
>>>    fr_sof(fp) = hp->fcoe_sof;
>>>    if (skb_copy_bits(skb, fr_len, &crc_eof, sizeof(crc_eof))) {
>>> -        put_cpu();
>>>        kfree_skb(skb);
>>>        return;
>>>    }
>>>    fr_eof(fp) = crc_eof.fcoe_eof;
>>>    fr_crc(fp) = crc_eof.fcoe_crc32;
>>>    if (pskb_trim(skb, fr_len)) {
>>> -        put_cpu();
>>>        kfree_skb(skb);
>>>        return;
>>>    }
>>> @@ -544,7 +538,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>>>        port = lport_priv(vn_port);
>>>        if (!ether_addr_equal(port->data_src_addr, dest_mac)) {
>>>            BNX2FC_HBA_DBG(lport, "fpma mismatch\n");
>>> -            put_cpu();
>>>            kfree_skb(skb);
>>>            return;
>>>        }
>>> @@ -552,7 +545,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>>>    if (fh->fh_r_ctl == FC_RCTL_DD_SOL_DATA &&
>>>        fh->fh_type == FC_TYPE_FCP) {
>>>        /* Drop FCP data. We dont this in L2 path */
>>> -        put_cpu();
>>>        kfree_skb(skb);
>>>        return;
>>>    }
>>> @@ -562,7 +554,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>>>        case ELS_LOGO:
>>>            if (ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
>>>                /* drop non-FIP LOGO */
>>> -                put_cpu();
>>>                kfree_skb(skb);
>>>                return;
>>>            }
>>> @@ -572,22 +563,23 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
>>> 
>>>    if (fh->fh_r_ctl == FC_RCTL_BA_ABTS) {
>>>        /* Drop incoming ABTS */
>>> -        put_cpu();
>>>        kfree_skb(skb);
>>>        return;
>>>    }
>>> 
>>> +    stats = per_cpu_ptr(lport->stats, smp_processor_id());
>>> +    stats->RxFrames++;
>>> +    stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
>>> +
>>>    if (le32_to_cpu(fr_crc(fp)) !=
>>>            ~crc32(~0, skb->data, fr_len)) {
>>>        if (stats->InvalidCRCCount < 5)
>>>            printk(KERN_WARNING PFX "dropping frame with "
>>>                   "CRC error\n");
>>>        stats->InvalidCRCCount++;
>>> -        put_cpu();
>>>        kfree_skb(skb);
>>>        return;
>>>    }
>>> -    put_cpu();
>>>    fc_exch_recv(lport, fp);
>>> }
>> 
>> 
>> 

Re: [PATCH] bnx2fc: Improve stats update mechanism

[Neil Horman]

On Sat, May 31, 2014 at 05:13:11AM +0000, Eddie Wai wrote:
> 
> 
> On May 30, 2014, at 7:48 PM, "Neil Horman" <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> 
> > On Fri, May 30, 2014 at 02:59:43PM -0700, Eddie Wai wrote:
> >> Thanks for fixing this.  The patch generally looks good, but I do have a
> >> few comments.
> >> 
> >> On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
> >>> Recently had this warning reported:
> >>> 
> >>> [  290.489047] Call Trace:
> >>> [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
> >>> [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
> >>> [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
> >>> [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
> >>> [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
> >>> [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
> >>> [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
> >>> [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
> >>> [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> >>> [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
> >>> [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> >>> 
> >>> Its due to the fact that we call a potentially sleeping function from the bnx2fc
> >>> rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
> >>> variable stats lookup in bnx2fc_l2_rcv_thread.
> >>> 
> >>> Easy enough fix, we can just move the stats collection later in the function
> >>> where we are sure we won't preempt or sleep.  This also allows us to not have to
> >>> enable pre-emption when doing a per-cpu lookup, since we're certain not to get
> >> You mean this allows us to not have to 'disable' pre-emption, right? 
> >> 
> >> Can you elaborate on how we can be sure that we won't get preempted
> >> immediately after retrieving the CPU variable?  I would think it'll be
> >> okay to call get_cpu at this stage as there won't be any sleepable mutex
> >> lock calls before the put_cpu.
> > We can't be sure, but I would assert it doesn't really matter at this point.
> > The area in which we update stats is so small that, even if we do hit the
> > unlikely chance that we get pre-empted, and then rescheduled on a different cpu,
> > it won't matter.  We could add the get_cpu/put_cpu back if you're really intent
> > on it, but I'm not sure its worthwhile given that this is a hot path.
> I agree with your assessment.  But code wise just so bnx2fc is consistent to the software FCoE counterpart, I would advice to use the same get_cpu to retrieve that stats CPU variable.  Thanks.

Actually I woke up this morning meaning to send a follow on note addressing
that.  The Soft FCoE counterpart to this function acutally works the way bnx2fc
reception does after my patch here.  Thats because the stats are updated with
the cpu held, but the frame is actually received by the fc layer immediately
after cpu_put is called.  The implication is that the task can get preempted
right after we update the stats and re-enable preemption, meaning that we may
actually receive the frame on a different cpu than the cpu we updated the stats
on.  I was planning on sending a patch to switch get_cpu/put_cpu in the soft
FCoE code to just use smp_processor_id(), since it doesn't help anything.

bnx2fc is actually in a slightly better position than soft FCoE.  soft FCoE
creates a thread for each cpu, but doesn't explicitly assign single cpu affinity
for each task, meaning per-cpu stats are actually relevant.  For bnx2fc, you
only create a single task at module init, meaning there is no parallel reception
of frames.  As such the per-cpu tasks are really more of an aggregate measure
(since the stat updates are all serialized anyway by the single thread accross
cpus).

The bottom line is that you can't hold a cpu while both doing the work of frame
reception and updating statistics, unless you want to avoid sleeping functions
in the entire receive path, and once you separate the two by only holding the
cpu during stats update, you run the risk of changing the cpu after you've
processed the frame, but before you dereference the per_cpu pointer.

I can still re-add the get_cpu/put_cpu if you want, but I really don't see the
purpose of the extra overhead given the above, and my intention is to remove it
from the soft FCoE code as well.

Regards
Neil

Re: [PATCH] bnx2fc: Improve stats update mechanism

[Neil Horman]

On Sat, May 31, 2014 at 08:37:57AM -0400, Neil Horman wrote:
> On Sat, May 31, 2014 at 05:13:11AM +0000, Eddie Wai wrote:
> > 
> > 
> > On May 30, 2014, at 7:48 PM, "Neil Horman" <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> > 
> > > On Fri, May 30, 2014 at 02:59:43PM -0700, Eddie Wai wrote:
> > >> Thanks for fixing this.  The patch generally looks good, but I do have a
> > >> few comments.
> > >> 
> > >> On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
> > >>> Recently had this warning reported:
> > >>> 
> > >>> [  290.489047] Call Trace:
> > >>> [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
> > >>> [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
> > >>> [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
> > >>> [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
> > >>> [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
> > >>> [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
> > >>> [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
> > >>> [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
> > >>> [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > >>> [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
> > >>> [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > >>> 
> > >>> Its due to the fact that we call a potentially sleeping function from the bnx2fc
> > >>> rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
> > >>> variable stats lookup in bnx2fc_l2_rcv_thread.
> > >>> 
> > >>> Easy enough fix, we can just move the stats collection later in the function
> > >>> where we are sure we won't preempt or sleep.  This also allows us to not have to
> > >>> enable pre-emption when doing a per-cpu lookup, since we're certain not to get
> > >> You mean this allows us to not have to 'disable' pre-emption, right? 
> > >> 
> > >> Can you elaborate on how we can be sure that we won't get preempted
> > >> immediately after retrieving the CPU variable?  I would think it'll be
> > >> okay to call get_cpu at this stage as there won't be any sleepable mutex
> > >> lock calls before the put_cpu.
> > > We can't be sure, but I would assert it doesn't really matter at this point.
> > > The area in which we update stats is so small that, even if we do hit the
> > > unlikely chance that we get pre-empted, and then rescheduled on a different cpu,
> > > it won't matter.  We could add the get_cpu/put_cpu back if you're really intent
> > > on it, but I'm not sure its worthwhile given that this is a hot path.
> > I agree with your assessment.  But code wise just so bnx2fc is consistent to the software FCoE counterpart, I would advice to use the same get_cpu to retrieve that stats CPU variable.  Thanks.
> 
> Actually I woke up this morning meaning to send a follow on note addressing
> that.  The Soft FCoE counterpart to this function acutally works the way bnx2fc
> reception does after my patch here.  Thats because the stats are updated with
> the cpu held, but the frame is actually received by the fc layer immediately
> after cpu_put is called.  The implication is that the task can get preempted
> right after we update the stats and re-enable preemption, meaning that we may
> actually receive the frame on a different cpu than the cpu we updated the stats
> on.  I was planning on sending a patch to switch get_cpu/put_cpu in the soft
> FCoE code to just use smp_processor_id(), since it doesn't help anything.
> 
> bnx2fc is actually in a slightly better position than soft FCoE.  soft FCoE
> creates a thread for each cpu, but doesn't explicitly assign single cpu affinity
> for each task, meaning per-cpu stats are actually relevant.  For bnx2fc, you
> only create a single task at module init, meaning there is no parallel reception
> of frames.  As such the per-cpu tasks are really more of an aggregate measure
> (since the stat updates are all serialized anyway by the single thread accross
> cpus).
> 
> The bottom line is that you can't hold a cpu while both doing the work of frame
> reception and updating statistics, unless you want to avoid sleeping functions
> in the entire receive path, and once you separate the two by only holding the
> cpu during stats update, you run the risk of changing the cpu after you've
> processed the frame, but before you dereference the per_cpu pointer.
> 
> I can still re-add the get_cpu/put_cpu if you want, but I really don't see the
> purpose of the extra overhead given the above, and my intention is to remove it
> from the soft FCoE code as well.
> 
> Regards
> Neil
> 
Can I get further comment or an ACK on this so the fcoe tree can pull it in?
Thanks!
Neil

> _______________________________________________
> fcoe-devel mailing list
> fcoe-devel-s9riP+hp16TNLxjTenLetw@public.gmane.org
> http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
> 

Re: [PATCH] bnx2fc: Improve stats update mechanism

[Eddie Wai]

On Tue, 2014-06-03 at 10:54 -0400, Neil Horman wrote:
> On Sat, May 31, 2014 at 08:37:57AM -0400, Neil Horman wrote:
> > On Sat, May 31, 2014 at 05:13:11AM +0000, Eddie Wai wrote:
> > > 
> > > 
> > > On May 30, 2014, at 7:48 PM, "Neil Horman" <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> > > 
> > > > On Fri, May 30, 2014 at 02:59:43PM -0700, Eddie Wai wrote:
> > > >> Thanks for fixing this.  The patch generally looks good, but I do have a
> > > >> few comments.
> > > >> 
> > > >> On Fri, 2014-05-30 at 11:01 -0400, Neil Horman wrote:
> > > >>> Recently had this warning reported:
> > > >>> 
> > > >>> [  290.489047] Call Trace:
> > > >>> [  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
> > > >>> [  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
> > > >>> [  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
> > > >>> [  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
> > > >>> [  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
> > > >>> [  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
> > > >>> [  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
> > > >>> [  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
> > > >>> [  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > > >>> [  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
> > > >>> [  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
> > > >>> 
> > > >>> Its due to the fact that we call a potentially sleeping function from the bnx2fc
> > > >>> rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
> > > >>> variable stats lookup in bnx2fc_l2_rcv_thread.
> > > >>> 
> > > >>> Easy enough fix, we can just move the stats collection later in the function
> > > >>> where we are sure we won't preempt or sleep.  This also allows us to not have to
> > > >>> enable pre-emption when doing a per-cpu lookup, since we're certain not to get
> > > >> You mean this allows us to not have to 'disable' pre-emption, right? 
> > > >> 
> > > >> Can you elaborate on how we can be sure that we won't get preempted
> > > >> immediately after retrieving the CPU variable?  I would think it'll be
> > > >> okay to call get_cpu at this stage as there won't be any sleepable mutex
> > > >> lock calls before the put_cpu.
> > > > We can't be sure, but I would assert it doesn't really matter at this point.
> > > > The area in which we update stats is so small that, even if we do hit the
> > > > unlikely chance that we get pre-empted, and then rescheduled on a different cpu,
> > > > it won't matter.  We could add the get_cpu/put_cpu back if you're really intent
> > > > on it, but I'm not sure its worthwhile given that this is a hot path.
> > > I agree with your assessment.  But code wise just so bnx2fc is consistent to the software FCoE counterpart, I would advice to use the same get_cpu to retrieve that stats CPU variable.  Thanks.
> > 
> > Actually I woke up this morning meaning to send a follow on note addressing
> > that.  The Soft FCoE counterpart to this function acutally works the way bnx2fc
> > reception does after my patch here.  Thats because the stats are updated with
> > the cpu held, but the frame is actually received by the fc layer immediately
> > after cpu_put is called.  The implication is that the task can get preempted
> > right after we update the stats and re-enable preemption, meaning that we may
> > actually receive the frame on a different cpu than the cpu we updated the stats
> > on.  I was planning on sending a patch to switch get_cpu/put_cpu in the soft
> > FCoE code to just use smp_processor_id(), since it doesn't help anything.
> > 
> > bnx2fc is actually in a slightly better position than soft FCoE.  soft FCoE
> > creates a thread for each cpu, but doesn't explicitly assign single cpu affinity
> > for each task, meaning per-cpu stats are actually relevant.  For bnx2fc, you
> > only create a single task at module init, meaning there is no parallel reception
> > of frames.  As such the per-cpu tasks are really more of an aggregate measure
> > (since the stat updates are all serialized anyway by the single thread accross
> > cpus).
That's correct.  bnx2fc uses only a single thread for this L2 recv path
because fast path traffic normally goes to the offload path.
> > 
> > The bottom line is that you can't hold a cpu while both doing the work of frame
> > reception and updating statistics, unless you want to avoid sleeping functions
> > in the entire receive path, and once you separate the two by only holding the
> > cpu during stats update, you run the risk of changing the cpu after you've
> > processed the frame, but before you dereference the per_cpu pointer.
> > 
> > I can still re-add the get_cpu/put_cpu if you want, but I really don't see the
> > purpose of the extra overhead given the above, and my intention is to remove it
> > from the soft FCoE code as well.
Nah, just as long as we are consistent, that's good enough for me!
Thanks.
> > 
> > Regards
> > Neil
> > 
> Can I get further comment or an ACK on this so the fcoe tree can pull it in?
Acked-by:  Eddie Wai <eddie.wai-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Thanks!
> Neil
> 
> > _______________________________________________
> > fcoe-devel mailing list
> > fcoe-devel-s9riP+hp16TNLxjTenLetw@public.gmane.org
> > http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
> > 

[PATCH] bnx2fc: Improve stats update mechanism

[Neil Horman]

Recently had this warning reported:

[  290.489047] Call Trace:
[  290.489053]  [<ffffffff8169efec>] dump_stack+0x19/0x1b
[  290.489055]  [<ffffffff810ac7a9>] __might_sleep+0x179/0x230
[  290.489057]  [<ffffffff816a4ad5>] mutex_lock_nested+0x55/0x520
[  290.489061]  [<ffffffffa01b9905>] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
[  290.489065]  [<ffffffffa0174c1a>] fc_vport_id_lookup+0x3a/0xa0 [libfc]
[  290.489068]  [<ffffffffa01b9a6c>] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
[  290.489070]  [<ffffffffa01b9840>] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
[  290.489073]  [<ffffffff8109e0cd>] kthread+0xed/0x100
[  290.489075]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80
[  290.489077]  [<ffffffff816b2fec>] ret_from_fork+0x7c/0xb0
[  290.489078]  [<ffffffff8109dfe0>] ? insert_kthread_work+0x80/0x80

Its due to the fact that we call a potentially sleeping function from the bnx2fc
rcv path with preemption disabled (via the get_cpu call embedded in the per-cpu
variable stats lookup in bnx2fc_l2_rcv_thread.

Easy enough fix, we can just move the stats collection later in the function
where we are sure we won't preempt or sleep.  This also allows us to not have to
enable pre-emption when doing a per-cpu lookup, since we're certain not to get
rescheduled.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "James E.J. Bottomley" <JBottomley@parallels.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: linux-scsi@vger.kernel.org
CC: Robert Love <robert.w.love@intel.com>
CC: Vasu Dev <vasu.dev@intel.com>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 9b94850..475eee5 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -516,23 +516,17 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 	skb_pull(skb, sizeof(struct fcoe_hdr));
 	fr_len = skb->len - sizeof(struct fcoe_crc_eof);
 
-	stats = per_cpu_ptr(lport->stats, get_cpu());
-	stats->RxFrames++;
-	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
-
 	fp = (struct fc_frame *)skb;
 	fc_frame_init(fp);
 	fr_dev(fp) = lport;
 	fr_sof(fp) = hp->fcoe_sof;
 	if (skb_copy_bits(skb, fr_len, &crc_eof, sizeof(crc_eof))) {
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
 	fr_eof(fp) = crc_eof.fcoe_eof;
 	fr_crc(fp) = crc_eof.fcoe_crc32;
 	if (pskb_trim(skb, fr_len)) {
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
@@ -544,7 +538,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 		port = lport_priv(vn_port);
 		if (!ether_addr_equal(port->data_src_addr, dest_mac)) {
 			BNX2FC_HBA_DBG(lport, "fpma mismatch\n");
-			put_cpu();
 			kfree_skb(skb);
 			return;
 		}
@@ -552,7 +545,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 	if (fh->fh_r_ctl == FC_RCTL_DD_SOL_DATA &&
 	    fh->fh_type == FC_TYPE_FCP) {
 		/* Drop FCP data. We dont this in L2 path */
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
@@ -562,7 +554,6 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 		case ELS_LOGO:
 			if (ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
 				/* drop non-FIP LOGO */
-				put_cpu();
 				kfree_skb(skb);
 				return;
 			}
@@ -572,22 +563,23 @@ static void bnx2fc_recv_frame(struct sk_buff *skb)
 
 	if (fh->fh_r_ctl == FC_RCTL_BA_ABTS) {
 		/* Drop incoming ABTS */
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
 
+	stats = per_cpu_ptr(lport->stats, smp_processor_id());
+	stats->RxFrames++;
+	stats->RxWords += fr_len / FCOE_WORD_TO_BYTE;
+
 	if (le32_to_cpu(fr_crc(fp)) !=
 			~crc32(~0, skb->data, fr_len)) {
 		if (stats->InvalidCRCCount < 5)
 			printk(KERN_WARNING PFX "dropping frame with "
 			       "CRC error\n");
 		stats->InvalidCRCCount++;
-		put_cpu();
 		kfree_skb(skb);
 		return;
 	}
-	put_cpu();
 	fc_exch_recv(lport, fp);
 }
 
-- 
1.8.3.1