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 <nhor...@tuxdriver.com>
> CC: linux-scsi@vger.kernel.org
> CC: Robert Love <robert.w.l...@intel.com>
> CC: Vasu Dev <vasu....@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);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
