Re: Observing Softlockup's while running heavy IOs

2016-09-12 Thread Neil Horman 
On Mon, Sep 12, 2016 at 01:48:39PM +0530, Sreekanth Reddy wrote:
> On Thu, Sep 8, 2016 at 7:09 PM, Neil Horman <nhor...@tuxdriver.com> wrote:
> > On Thu, Sep 08, 2016 at 11:12:40AM +0530, Sreekanth Reddy wrote:
> >> On Wed, Sep 7, 2016 at 6:54 PM, Neil Horman <nhor...@tuxdriver.com> wrote:
> >> > On Wed, Sep 07, 2016 at 11:30:04AM +0530, Sreekanth Reddy wrote:
> >> >> On Tue, Sep 6, 2016 at 8:36 PM, Neil Horman <nhor...@tuxdriver.com> 
> >> >> wrote:
> >> >> > On Tue, Sep 06, 2016 at 04:52:37PM +0530, Sreekanth Reddy wrote:
> >> >> >> On Fri, Sep 2, 2016 at 4:34 AM, Bart Van Assche
> >> >> >> <bart.vanass...@sandisk.com> wrote:
> >> >> >> > On 09/01/2016 03:31 AM, Sreekanth Reddy wrote:
> >> >> >> >>
> >> >> >> >> I reduced the ISR workload by one third in-order to reduce the 
> >> >> >> >> time
> >> >> >> >> that is spent per CPU in interrupt context, even then I am 
> >> >> >> >> observing
> >> >> >> >> softlockups.
> >> >> >> >>
> >> >> >> >> As I mentioned before only same single CPU in the set of 
> >> >> >> >> CPUs(enabled
> >> >> >> >> in affinity_hint) is busy with handling the interrupts from
> >> >> >> >> corresponding IRQx. I have done below experiment in driver to 
> >> >> >> >> limit
> >> >> >> >> these softlockups/hardlockups. But I am not sure whether it is
> >> >> >> >> reasonable to do this in driver,
> >> >> >> >>
> >> >> >> >> Experiment:
> >> >> >> >> If the CPUx is continuously busy with handling the remote CPUs
> >> >> >> >> (enabled in the corresponding IRQ's affinity_hint) IO works by 
> >> >> >> >> 1/4th
> >> >> >> >> of the HBA queue depth in the same ISR context then enable a flag
> >> >> >> >> called 'change_smp_affinity' for this IRQ. Also created a thread 
> >> >> >> >> with
> >> >> >> >> will poll for this flag for every IRQ's (enabled by driver) for 
> >> >> >> >> every
> >> >> >> >> second. If this thread see that this flag is enabled for any IRQ 
> >> >> >> >> then
> >> >> >> >> it will write next CPU number from the CPUs enabled in the IRQ's
> >> >> >> >> affinity_hint to the IRQ's smp_affinity procfs attribute using
> >> >> >> >> 'call_usermodehelper()' API.
> >> >> >> >>
> >> >> >> >> This to make sure that interrupts are not processed by same 
> >> >> >> >> single CPU
> >> >> >> >> all the time and to make the other CPUs to handle the interrupts 
> >> >> >> >> if
> >> >> >> >> the current CPU is continuously busy with handling the other CPUs 
> >> >> >> >> IO
> >> >> >> >> interrupts.
> >> >> >> >>
> >> >> >> >> For example consider a system which has 8 logical CPUs and one 
> >> >> >> >> MSIx
> >> >> >> >> vector enabled (called IRQ 120) in driver, HBA queue depth as 8K.
> >> >> >> >> then IRQ's procfs attributes will be
> >> >> >> >> IRQ# 120, affinity_hint=0xff, smp_affinity=0x00
> >> >> >> >>
> >> >> >> >> After starting heavy IOs, we will observe that only CPU0 will be 
> >> >> >> >> busy
> >> >> >> >> with handling the interrupts. This experiment driver will change 
> >> >> >> >> the
> >> >> >> >> smp_affinity to next CPU number i.e. 0x01 (using cmd 'echo 0x01 >
> >> >> >> >> /proc/irq/120/smp_affinity', driver issue's this cmd using
> >> >> >> >> call_usermodehelper() API) if it observes that CPU0 is 
> >> >> >> >> continuously
> >> >> >> >> processing more than 2K of IOs replies of other CPUs i.e from 
> >> >> >> >> CPU1 to
> >> >> >> >>

Re: Observing Softlockup's while running heavy IOs

2016-09-08 Thread Neil Horman 
On Thu, Sep 08, 2016 at 11:12:40AM +0530, Sreekanth Reddy wrote:
> On Wed, Sep 7, 2016 at 6:54 PM, Neil Horman <nhor...@tuxdriver.com> wrote:
> > On Wed, Sep 07, 2016 at 11:30:04AM +0530, Sreekanth Reddy wrote:
> >> On Tue, Sep 6, 2016 at 8:36 PM, Neil Horman <nhor...@tuxdriver.com> wrote:
> >> > On Tue, Sep 06, 2016 at 04:52:37PM +0530, Sreekanth Reddy wrote:
> >> >> On Fri, Sep 2, 2016 at 4:34 AM, Bart Van Assche
> >> >> <bart.vanass...@sandisk.com> wrote:
> >> >> > On 09/01/2016 03:31 AM, Sreekanth Reddy wrote:
> >> >> >>
> >> >> >> I reduced the ISR workload by one third in-order to reduce the time
> >> >> >> that is spent per CPU in interrupt context, even then I am observing
> >> >> >> softlockups.
> >> >> >>
> >> >> >> As I mentioned before only same single CPU in the set of CPUs(enabled
> >> >> >> in affinity_hint) is busy with handling the interrupts from
> >> >> >> corresponding IRQx. I have done below experiment in driver to limit
> >> >> >> these softlockups/hardlockups. But I am not sure whether it is
> >> >> >> reasonable to do this in driver,
> >> >> >>
> >> >> >> Experiment:
> >> >> >> If the CPUx is continuously busy with handling the remote CPUs
> >> >> >> (enabled in the corresponding IRQ's affinity_hint) IO works by 1/4th
> >> >> >> of the HBA queue depth in the same ISR context then enable a flag
> >> >> >> called 'change_smp_affinity' for this IRQ. Also created a thread with
> >> >> >> will poll for this flag for every IRQ's (enabled by driver) for every
> >> >> >> second. If this thread see that this flag is enabled for any IRQ then
> >> >> >> it will write next CPU number from the CPUs enabled in the IRQ's
> >> >> >> affinity_hint to the IRQ's smp_affinity procfs attribute using
> >> >> >> 'call_usermodehelper()' API.
> >> >> >>
> >> >> >> This to make sure that interrupts are not processed by same single 
> >> >> >> CPU
> >> >> >> all the time and to make the other CPUs to handle the interrupts if
> >> >> >> the current CPU is continuously busy with handling the other CPUs IO
> >> >> >> interrupts.
> >> >> >>
> >> >> >> For example consider a system which has 8 logical CPUs and one MSIx
> >> >> >> vector enabled (called IRQ 120) in driver, HBA queue depth as 8K.
> >> >> >> then IRQ's procfs attributes will be
> >> >> >> IRQ# 120, affinity_hint=0xff, smp_affinity=0x00
> >> >> >>
> >> >> >> After starting heavy IOs, we will observe that only CPU0 will be busy
> >> >> >> with handling the interrupts. This experiment driver will change the
> >> >> >> smp_affinity to next CPU number i.e. 0x01 (using cmd 'echo 0x01 >
> >> >> >> /proc/irq/120/smp_affinity', driver issue's this cmd using
> >> >> >> call_usermodehelper() API) if it observes that CPU0 is continuously
> >> >> >> processing more than 2K of IOs replies of other CPUs i.e from CPU1 to
> >> >> >> CPU7.
> >> >> >>
> >> >> >> Whether doing this kind of stuff in driver is ok?
> >> >> >
> >> >> >
> >> >> > Hello Sreekanth,
> >> >> >
> >> >> > To me this sounds like something that should be implemented in the I/O
> >> >> > chipset on the motherboard. If you have a look at the Intel Software
> >> >> > Developer Manuals then you will see that logical destination mode 
> >> >> > supports
> >> >> > round-robin interrupt delivery. However, the Linux kernel selects 
> >> >> > physical
> >> >> > destination mode on systems with more than eight logical CPUs (see 
> >> >> > also
> >> >> > arch/x86/kernel/apic/apic_flat_64.c).
> >> >> >
> >> >> > I'm not sure the maintainers of the interrupt subsystem would welcome 
> >> >> > code
> >> >> > that emulates round-robin interrupt delivery. So your best option is
> >> >> > probably to minimize the amo

Re: Observing Softlockup's while running heavy IOs

2016-09-07 Thread Neil Horman 
On Wed, Sep 07, 2016 at 11:30:04AM +0530, Sreekanth Reddy wrote:
> On Tue, Sep 6, 2016 at 8:36 PM, Neil Horman <nhor...@tuxdriver.com> wrote:
> > On Tue, Sep 06, 2016 at 04:52:37PM +0530, Sreekanth Reddy wrote:
> >> On Fri, Sep 2, 2016 at 4:34 AM, Bart Van Assche
> >> <bart.vanass...@sandisk.com> wrote:
> >> > On 09/01/2016 03:31 AM, Sreekanth Reddy wrote:
> >> >>
> >> >> I reduced the ISR workload by one third in-order to reduce the time
> >> >> that is spent per CPU in interrupt context, even then I am observing
> >> >> softlockups.
> >> >>
> >> >> As I mentioned before only same single CPU in the set of CPUs(enabled
> >> >> in affinity_hint) is busy with handling the interrupts from
> >> >> corresponding IRQx. I have done below experiment in driver to limit
> >> >> these softlockups/hardlockups. But I am not sure whether it is
> >> >> reasonable to do this in driver,
> >> >>
> >> >> Experiment:
> >> >> If the CPUx is continuously busy with handling the remote CPUs
> >> >> (enabled in the corresponding IRQ's affinity_hint) IO works by 1/4th
> >> >> of the HBA queue depth in the same ISR context then enable a flag
> >> >> called 'change_smp_affinity' for this IRQ. Also created a thread with
> >> >> will poll for this flag for every IRQ's (enabled by driver) for every
> >> >> second. If this thread see that this flag is enabled for any IRQ then
> >> >> it will write next CPU number from the CPUs enabled in the IRQ's
> >> >> affinity_hint to the IRQ's smp_affinity procfs attribute using
> >> >> 'call_usermodehelper()' API.
> >> >>
> >> >> This to make sure that interrupts are not processed by same single CPU
> >> >> all the time and to make the other CPUs to handle the interrupts if
> >> >> the current CPU is continuously busy with handling the other CPUs IO
> >> >> interrupts.
> >> >>
> >> >> For example consider a system which has 8 logical CPUs and one MSIx
> >> >> vector enabled (called IRQ 120) in driver, HBA queue depth as 8K.
> >> >> then IRQ's procfs attributes will be
> >> >> IRQ# 120, affinity_hint=0xff, smp_affinity=0x00
> >> >>
> >> >> After starting heavy IOs, we will observe that only CPU0 will be busy
> >> >> with handling the interrupts. This experiment driver will change the
> >> >> smp_affinity to next CPU number i.e. 0x01 (using cmd 'echo 0x01 >
> >> >> /proc/irq/120/smp_affinity', driver issue's this cmd using
> >> >> call_usermodehelper() API) if it observes that CPU0 is continuously
> >> >> processing more than 2K of IOs replies of other CPUs i.e from CPU1 to
> >> >> CPU7.
> >> >>
> >> >> Whether doing this kind of stuff in driver is ok?
> >> >
> >> >
> >> > Hello Sreekanth,
> >> >
> >> > To me this sounds like something that should be implemented in the I/O
> >> > chipset on the motherboard. If you have a look at the Intel Software
> >> > Developer Manuals then you will see that logical destination mode 
> >> > supports
> >> > round-robin interrupt delivery. However, the Linux kernel selects 
> >> > physical
> >> > destination mode on systems with more than eight logical CPUs (see also
> >> > arch/x86/kernel/apic/apic_flat_64.c).
> >> >
> >> > I'm not sure the maintainers of the interrupt subsystem would welcome 
> >> > code
> >> > that emulates round-robin interrupt delivery. So your best option is
> >> > probably to minimize the amount of work that is done in interrupt context
> >> > and to move as much work as possible out of interrupt context in such a 
> >> > way
> >> > that it can be spread over multiple CPU cores, e.g. by using
> >> > queue_work_on().
> >> >
> >> > Bart.
> >>
> >> Bart,
> >>
> >> Thanks a lot for providing lot of inputs and valuable information on this 
> >> issue.
> >>
> >> Today I got one more observation. i.e. I am not observing any lockups
> >> if I use 1.0.4-6 versioned irqbalance.
> >> Since this versioned irqbalance is able to shift the load to other CPU
> >> when one CPU is heavily loaded.
> >>
> >
> > This isn

Re: Observing Softlockup's while running heavy IOs

2016-09-06 Thread Neil Horman 
On Tue, Sep 06, 2016 at 04:52:37PM +0530, Sreekanth Reddy wrote:
> On Fri, Sep 2, 2016 at 4:34 AM, Bart Van Assche
>  wrote:
> > On 09/01/2016 03:31 AM, Sreekanth Reddy wrote:
> >>
> >> I reduced the ISR workload by one third in-order to reduce the time
> >> that is spent per CPU in interrupt context, even then I am observing
> >> softlockups.
> >>
> >> As I mentioned before only same single CPU in the set of CPUs(enabled
> >> in affinity_hint) is busy with handling the interrupts from
> >> corresponding IRQx. I have done below experiment in driver to limit
> >> these softlockups/hardlockups. But I am not sure whether it is
> >> reasonable to do this in driver,
> >>
> >> Experiment:
> >> If the CPUx is continuously busy with handling the remote CPUs
> >> (enabled in the corresponding IRQ's affinity_hint) IO works by 1/4th
> >> of the HBA queue depth in the same ISR context then enable a flag
> >> called 'change_smp_affinity' for this IRQ. Also created a thread with
> >> will poll for this flag for every IRQ's (enabled by driver) for every
> >> second. If this thread see that this flag is enabled for any IRQ then
> >> it will write next CPU number from the CPUs enabled in the IRQ's
> >> affinity_hint to the IRQ's smp_affinity procfs attribute using
> >> 'call_usermodehelper()' API.
> >>
> >> This to make sure that interrupts are not processed by same single CPU
> >> all the time and to make the other CPUs to handle the interrupts if
> >> the current CPU is continuously busy with handling the other CPUs IO
> >> interrupts.
> >>
> >> For example consider a system which has 8 logical CPUs and one MSIx
> >> vector enabled (called IRQ 120) in driver, HBA queue depth as 8K.
> >> then IRQ's procfs attributes will be
> >> IRQ# 120, affinity_hint=0xff, smp_affinity=0x00
> >>
> >> After starting heavy IOs, we will observe that only CPU0 will be busy
> >> with handling the interrupts. This experiment driver will change the
> >> smp_affinity to next CPU number i.e. 0x01 (using cmd 'echo 0x01 >
> >> /proc/irq/120/smp_affinity', driver issue's this cmd using
> >> call_usermodehelper() API) if it observes that CPU0 is continuously
> >> processing more than 2K of IOs replies of other CPUs i.e from CPU1 to
> >> CPU7.
> >>
> >> Whether doing this kind of stuff in driver is ok?
> >
> >
> > Hello Sreekanth,
> >
> > To me this sounds like something that should be implemented in the I/O
> > chipset on the motherboard. If you have a look at the Intel Software
> > Developer Manuals then you will see that logical destination mode supports
> > round-robin interrupt delivery. However, the Linux kernel selects physical
> > destination mode on systems with more than eight logical CPUs (see also
> > arch/x86/kernel/apic/apic_flat_64.c).
> >
> > I'm not sure the maintainers of the interrupt subsystem would welcome code
> > that emulates round-robin interrupt delivery. So your best option is
> > probably to minimize the amount of work that is done in interrupt context
> > and to move as much work as possible out of interrupt context in such a way
> > that it can be spread over multiple CPU cores, e.g. by using
> > queue_work_on().
> >
> > Bart.
> 
> Bart,
> 
> Thanks a lot for providing lot of inputs and valuable information on this 
> issue.
> 
> Today I got one more observation. i.e. I am not observing any lockups
> if I use 1.0.4-6 versioned irqbalance.
> Since this versioned irqbalance is able to shift the load to other CPU
> when one CPU is heavily loaded.
> 

This isn't happening because irqbalance is no longer able to shift load between
cpus, its happening because of commit 996ee2cf7a4d10454de68ac4978adb5cf22850f8.
irqs with higher interrupt volumes sould be balanced to a specific cpu core,
rather than to a cache domain to maximize cpu-local cache hit rates.  Prior to
that change we balanced to a cache domain and your workload didn't have to
serialize multiple interrupts to a single core.  My suggestion to you is to use
the --policyscript option to make your storage irqs get balanced to the cache
level, rather than the core level.  That should return the behavior to what you
want.

Neil

> while running heavy IOs, for first few seconds here is my driver irq's
> attributes,
> 
> ioc number = 0
> number of core processors = 24
> msix vector count = 2
> number of cores per msix vector = 16
> 
> 
> msix index = 0, irq number =  50, smp_affinity = 40
> affinity_hint = 000fff
> msix index = 1, irq number =  51, smp_affinity = 001000
> affinity_hint = fff000
> 
> We have set affinity for 2 msix vectors and 24 core processors
> --
> 
> After few seconds it observed that CPU12 is heavily loaded for IRQ 51
> and it changed the smp_affinity to CPU21
>

Re: [Open-FCoE] [PATCH] fcoe: fix reset of fip selection time.

2016-02-29 Thread Neil Horman 
On Mon, Feb 29, 2016 at 03:36:52AM -0800, Usha Ketineni wrote:
> Do not reset fip selection time for every advertisement
> in fcoe_ctlr_recv_adv() but set it only once for the first
> validated FCF. Otherwise FCF selection won't happen when the
> advertisements consistently arrive with sub FCOE_CTLR_START_DELAY
> periodicity.
> 
> Tested-by: Narendra K <narendr...@dell.com>
> Acked-by: Neil Horman <nhor...@tuxdriver.com>
> Reviewed-by: Johannes Thumshirn <jthumsh...@suse.de>
> Acked-by: Vasu Dev <vasu@intel.com>
> Signed-off-by: Usha Ketineni <usha.k.ketin...@intel.com>
> ---
>  drivers/scsi/fcoe/fcoe_ctlr.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
> index d68d572..0c85ef6 100644
> --- a/drivers/scsi/fcoe/fcoe_ctlr.c
> +++ b/drivers/scsi/fcoe/fcoe_ctlr.c
> @@ -1079,7 +1079,8 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip, 
> struct sk_buff *skb)
>* If this is the first validated FCF, note the time and
>* set a timer to trigger selection.
>*/
> - if (mtu_valid && !fip->sel_fcf && fcoe_ctlr_fcf_usable(fcf)) {
> + if (mtu_valid && !fip->sel_fcf && !fip->sel_time &&
> + fcoe_ctlr_fcf_usable(fcf)) {
>   fip->sel_time = jiffies +
>   msecs_to_jiffies(FCOE_CTLR_START_DELAY);
>   if (!timer_pending(>timer) ||
> 
> ___
> fcoe-devel mailing list
> fcoe-de...@open-fcoe.org
> http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
> 
Acked-by: Neil Horman <nhor...@tuxdriver.com>

--
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

[PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-06-23 Thread Neil Horman 
debugfs caught this:
WARNING: at lib/debugobjects.c:260 debug_print_object+0x83/0xa0()
ODEBUG: free active (active state 0) object type: work_struct
hint: fc_scsi_scan_rport+0x0/0xd0 [scsi_transport_fc]
 CPU: 1 PID: 184 Comm: kworker/1:1 Tainted: GW
--   3.10.0-123.el7.x86_64.debug #1
Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
Workqueue: fc_wq_5 fc_rport_final_delete [scsi_transport_fc]
Call Trace:
[8169efec] dump_stack+0x19/0x1b
[8106cbd1] warn_slowpath_common+0x61/0x80
[8106cc4c] warn_slowpath_fmt+0x5c/0x80
[8133e003] debug_print_object+0x83/0xa0
[a04e2f40] ? fc_parse_wwn+0x100/0x100

[8133f23b] debug_check_no_obj_freed+0x22b/0x270
[a04e127e] ? fc_rport_dev_release+0x1e/0x30
[811db3e9] kfree+0xd9/0x2d0
[a04e127e] fc_rport_dev_release+0x1e/0x30
[81428032] device_release+0x32/0xa0
[8132701e] kobject_release+0x7e/0x1b0
[81326ed8] kobject_put+0x28/0x60
[81428397] put_device+0x17/0x20
[a04e5025] fc_rport_final_delete+0x165/0x210
[810959b0] process_one_work+0x220/0x710
[81095944] ? process_one_work+0x1b4/0x710
[81095fbb] worker_thread+0x11b/0x3a0
[81095ea0] ? process_one_work+0x710/0x710
[8109e0cd] kthread+0xed/0x100
[8109dfe0] ? insert_kthread_work+0x80/0x80
[816b2fec] ret_from_fork+0x7c/0xb0
[8109dfe0] ? insert_kthread_work+0x80/0x80

Seems to be because the scan_work work_struct might be active when the housing
fc_rport struct gets freed.  Ensure that we cancel it prior to freeing the rport

Signed-off-by: Neil Horman nhor...@tuxdriver.com
CC: James E.J. Bottomley jbottom...@parallels.com
CC: Christoph Hellwig h...@infradead.org
CC: linux-scsi@vger.kernel.org
CC: Robert Love robert.w.l...@intel.com
CC: Vasu Dev vasu@intel.com
---
 drivers/scsi/scsi_transport_fc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 4628fd5..5bd552c 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2548,6 +2548,7 @@ fc_rport_final_delete(struct work_struct *work)
fc_flush_devloss(shost);
if (!cancel_delayed_work(rport-dev_loss_work))
fc_flush_devloss(shost);
+   cancel_work_sync(rport-scan_work);
spin_lock_irqsave(shost-host_lock, flags);
rport-flags = ~FC_RPORT_DEVLOSS_PENDING;
}
-- 
1.8.3.1

--
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

[PATCH] bnx2fc: Improve stats update mechanism

2014-06-23 Thread Neil Horman 
Recently had this warning reported:

[  290.489047] Call Trace:
[  290.489053]  [8169efec] dump_stack+0x19/0x1b
[  290.489055]  [810ac7a9] __might_sleep+0x179/0x230
[  290.489057]  [816a4ad5] mutex_lock_nested+0x55/0x520
[  290.489061]  [a01b9905] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
[  290.489065]  [a0174c1a] fc_vport_id_lookup+0x3a/0xa0 [libfc]
[  290.489068]  [a01b9a6c] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
[  290.489070]  [a01b9840] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
[  290.489073]  [8109e0cd] kthread+0xed/0x100
[  290.489075]  [8109dfe0] ? insert_kthread_work+0x80/0x80
[  290.489077]  [816b2fec] ret_from_fork+0x7c/0xb0
[  290.489078]  [8109dfe0] ? 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 nhor...@tuxdriver.com
CC: James E.J. Bottomley jbottom...@parallels.com
CC: Christoph Hellwig h...@infradead.org
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);
 }
 
-- 
1.8.3.1

--
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

Re: [Open-FCoE] [PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-06-16 Thread Neil Horman 
On Mon, Jun 02, 2014 at 04:22:50PM -0700, Vasu Dev wrote:
 On Fri, 2014-05-30 at 10:59 -0400, Neil Horman wrote:
  debugfs caught this:
  WARNING: at lib/debugobjects.c:260 debug_print_object+0x83/0xa0()
  ODEBUG: free active (active state 0) object type: work_struct
  hint: fc_scsi_scan_rport+0x0/0xd0 [scsi_transport_fc]
   CPU: 1 PID: 184 Comm: kworker/1:1 Tainted: GW
  --   3.10.0-123.el7.x86_64.debug #1
  Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
  Workqueue: fc_wq_5 fc_rport_final_delete [scsi_transport_fc]
  Call Trace:
  [8169efec] dump_stack+0x19/0x1b
  [8106cbd1] warn_slowpath_common+0x61/0x80
  [8106cc4c] warn_slowpath_fmt+0x5c/0x80
  [8133e003] debug_print_object+0x83/0xa0
  [a04e2f40] ? fc_parse_wwn+0x100/0x100
  
  [8133f23b] debug_check_no_obj_freed+0x22b/0x270
  [a04e127e] ? fc_rport_dev_release+0x1e/0x30
  [811db3e9] kfree+0xd9/0x2d0
  [a04e127e] fc_rport_dev_release+0x1e/0x30
  [81428032] device_release+0x32/0xa0
  [8132701e] kobject_release+0x7e/0x1b0
  [81326ed8] kobject_put+0x28/0x60
  [81428397] put_device+0x17/0x20
  [a04e5025] fc_rport_final_delete+0x165/0x210
  [810959b0] process_one_work+0x220/0x710
  [81095944] ? process_one_work+0x1b4/0x710
  [81095fbb] worker_thread+0x11b/0x3a0
  [81095ea0] ? process_one_work+0x710/0x710
  [8109e0cd] kthread+0xed/0x100
  [8109dfe0] ? insert_kthread_work+0x80/0x80
  [816b2fec] ret_from_fork+0x7c/0xb0
  [8109dfe0] ? insert_kthread_work+0x80/0x80
  
  Seems to be because the scan_work work_struct might be active when the 
  housing
  fc_rport struct gets freed.  Ensure that we cancel it prior to freeing the 
  rport
  
  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/scsi_transport_fc.c | 1 +
   1 file changed, 1 insertion(+)
  
  diff --git a/drivers/scsi/scsi_transport_fc.c 
  b/drivers/scsi/scsi_transport_fc.c
  index 4628fd5..5bd552c 100644
  --- a/drivers/scsi/scsi_transport_fc.c
  +++ b/drivers/scsi/scsi_transport_fc.c
  @@ -2548,6 +2548,7 @@ fc_rport_final_delete(struct work_struct *work)
  fc_flush_devloss(shost);
  if (!cancel_delayed_work(rport-dev_loss_work))
  fc_flush_devloss(shost);
  +   cancel_work_sync(rport-scan_work);
 
 Make sense to ensure pending work canceled, adding James Smart for his
 ACK as transport FC class author. 
 
 
 Reviewed-by: Vasu Dev vasu@intel.com 
 
 
  spin_lock_irqsave(shost-host_lock, flags);
  rport-flags = ~FC_RPORT_DEVLOSS_PENDING;
  }
 
 
 
Ping James, I beleve Christoph is still waiting on a review from you here.

Neil

--
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

Re: [Open-FCoE] [PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-06-10 Thread Neil Horman 
On Tue, Jun 10, 2014 at 04:38:41AM -0700, Christoph Hellwig wrote:
 On Mon, Jun 09, 2014 at 03:16:37PM -0400, Neil Horman wrote:
   Given fcoe is quite mature now and its patches volume is very low, so
   getting its kernel patches directly to scsi subsystem should work fine
   and should be okay with James or Christophs to pull into scsi subsystem
   directly once I've my non-author signoff ACK there as described in this
   announcement at http://marc.info/?l=linux-scsim=140050839729415w=2 
   
   If no alternate suggestion or objection to this then I'll formally
   announce this on fcoe mailing list.
   
   However for any huges patches series bomb or RFCs, I'll request fcoe
   developers to send patches against scsi tree at fcoe devel list first
   and then if needed I can roll them up.
   
   Thanks,
   Vasu
   
  Copy that Vasu, Christoph, is that ok with you?
 
 That's fine with me.  It would help greatly if you could make sure
 all the paches get a review or two very quickly so I can just pick them
 up immediately after reviewing them.
 
 
Roger that, Vasu has already acked this.  I thought there was a second, but I'm
not sure, my mailbox is a bit messed up at the moment.

Sorry for not cc-ing you on this sooner, I thought it was going to go through
the FCoE tree initially

Best
Neil

--
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

Re: [Open-FCoE] [PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-06-09 Thread Neil Horman 
On Mon, Jun 09, 2014 at 11:09:43AM -0700, Vasu Dev wrote:
 On Fri, 2014-06-06 at 16:54 -0400, Neil Horman wrote:
  On Mon, Jun 02, 2014 at 04:22:50PM -0700, Vasu Dev wrote:
   On Fri, 2014-05-30 at 10:59 -0400, Neil Horman wrote:
debugfs caught this:
WARNING: at lib/debugobjects.c:260 debug_print_object+0x83/0xa0()
ODEBUG: free active (active state 0) object type: work_struct
hint: fc_scsi_scan_rport+0x0/0xd0 [scsi_transport_fc]
 CPU: 1 PID: 184 Comm: kworker/1:1 Tainted: GW
--   3.10.0-123.el7.x86_64.debug #1
Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
Workqueue: fc_wq_5 fc_rport_final_delete [scsi_transport_fc]
Call Trace:
[8169efec] dump_stack+0x19/0x1b
[8106cbd1] warn_slowpath_common+0x61/0x80
[8106cc4c] warn_slowpath_fmt+0x5c/0x80
[8133e003] debug_print_object+0x83/0xa0
[a04e2f40] ? fc_parse_wwn+0x100/0x100

[8133f23b] debug_check_no_obj_freed+0x22b/0x270
[a04e127e] ? fc_rport_dev_release+0x1e/0x30
[811db3e9] kfree+0xd9/0x2d0
[a04e127e] fc_rport_dev_release+0x1e/0x30
[81428032] device_release+0x32/0xa0
[8132701e] kobject_release+0x7e/0x1b0
[81326ed8] kobject_put+0x28/0x60
[81428397] put_device+0x17/0x20
[a04e5025] fc_rport_final_delete+0x165/0x210
[810959b0] process_one_work+0x220/0x710
[81095944] ? process_one_work+0x1b4/0x710
[81095fbb] worker_thread+0x11b/0x3a0
[81095ea0] ? process_one_work+0x710/0x710
[8109e0cd] kthread+0xed/0x100
[8109dfe0] ? insert_kthread_work+0x80/0x80
[816b2fec] ret_from_fork+0x7c/0xb0
[8109dfe0] ? insert_kthread_work+0x80/0x80

Seems to be because the scan_work work_struct might be active when the 
housing
fc_rport struct gets freed.  Ensure that we cancel it prior to freeing 
the rport

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/scsi_transport_fc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_transport_fc.c 
b/drivers/scsi/scsi_transport_fc.c
index 4628fd5..5bd552c 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2548,6 +2548,7 @@ fc_rport_final_delete(struct work_struct *work)
fc_flush_devloss(shost);
if (!cancel_delayed_work(rport-dev_loss_work))
fc_flush_devloss(shost);
+   cancel_work_sync(rport-scan_work);
   
   Make sense to ensure pending work canceled, adding James Smart for his
   ACK as transport FC class author. 
   
   
   Reviewed-by: Vasu Dev vasu@intel.com 
   
  Ping on this, Something just occured to me.  I was thinking (perhaps
  erroneously) that this would go through the FCoE tree, but I don't see that
  you've setup a tree yet vasu (and Rob's has been idle for 6 months).  Whats 
  the
  plan for this (and future) fcoe patchs.  Will you have a tree, or will we 
  send
  this through Christophs new scsi tree perhaps?
  
 
 Thanks Neil for bringing this, I and Rob also had off list discussion on
 this just last week. 
 
 Given fcoe is quite mature now and its patches volume is very low, so
 getting its kernel patches directly to scsi subsystem should work fine
 and should be okay with James or Christophs to pull into scsi subsystem
 directly once I've my non-author signoff ACK there as described in this
 announcement at http://marc.info/?l=linux-scsim=140050839729415w=2 
 
 If no alternate suggestion or objection to this then I'll formally
 announce this on fcoe mailing list.
 
 However for any huges patches series bomb or RFCs, I'll request fcoe
 developers to send patches against scsi tree at fcoe devel list first
 and then if needed I can roll them up.
 
 Thanks,
 Vasu
 
Copy that Vasu, Christoph, is that ok with you?

Neil

 
 
 
 
 
 
 
--
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

Re: [Open-FCoE] [PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-06-06 Thread Neil Horman 
On Mon, Jun 02, 2014 at 04:22:50PM -0700, Vasu Dev wrote:
 On Fri, 2014-05-30 at 10:59 -0400, Neil Horman wrote:
  debugfs caught this:
  WARNING: at lib/debugobjects.c:260 debug_print_object+0x83/0xa0()
  ODEBUG: free active (active state 0) object type: work_struct
  hint: fc_scsi_scan_rport+0x0/0xd0 [scsi_transport_fc]
   CPU: 1 PID: 184 Comm: kworker/1:1 Tainted: GW
  --   3.10.0-123.el7.x86_64.debug #1
  Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
  Workqueue: fc_wq_5 fc_rport_final_delete [scsi_transport_fc]
  Call Trace:
  [8169efec] dump_stack+0x19/0x1b
  [8106cbd1] warn_slowpath_common+0x61/0x80
  [8106cc4c] warn_slowpath_fmt+0x5c/0x80
  [8133e003] debug_print_object+0x83/0xa0
  [a04e2f40] ? fc_parse_wwn+0x100/0x100
  
  [8133f23b] debug_check_no_obj_freed+0x22b/0x270
  [a04e127e] ? fc_rport_dev_release+0x1e/0x30
  [811db3e9] kfree+0xd9/0x2d0
  [a04e127e] fc_rport_dev_release+0x1e/0x30
  [81428032] device_release+0x32/0xa0
  [8132701e] kobject_release+0x7e/0x1b0
  [81326ed8] kobject_put+0x28/0x60
  [81428397] put_device+0x17/0x20
  [a04e5025] fc_rport_final_delete+0x165/0x210
  [810959b0] process_one_work+0x220/0x710
  [81095944] ? process_one_work+0x1b4/0x710
  [81095fbb] worker_thread+0x11b/0x3a0
  [81095ea0] ? process_one_work+0x710/0x710
  [8109e0cd] kthread+0xed/0x100
  [8109dfe0] ? insert_kthread_work+0x80/0x80
  [816b2fec] ret_from_fork+0x7c/0xb0
  [8109dfe0] ? insert_kthread_work+0x80/0x80
  
  Seems to be because the scan_work work_struct might be active when the 
  housing
  fc_rport struct gets freed.  Ensure that we cancel it prior to freeing the 
  rport
  
  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/scsi_transport_fc.c | 1 +
   1 file changed, 1 insertion(+)
  
  diff --git a/drivers/scsi/scsi_transport_fc.c 
  b/drivers/scsi/scsi_transport_fc.c
  index 4628fd5..5bd552c 100644
  --- a/drivers/scsi/scsi_transport_fc.c
  +++ b/drivers/scsi/scsi_transport_fc.c
  @@ -2548,6 +2548,7 @@ fc_rport_final_delete(struct work_struct *work)
  fc_flush_devloss(shost);
  if (!cancel_delayed_work(rport-dev_loss_work))
  fc_flush_devloss(shost);
  +   cancel_work_sync(rport-scan_work);
 
 Make sense to ensure pending work canceled, adding James Smart for his
 ACK as transport FC class author. 
 
 
 Reviewed-by: Vasu Dev vasu@intel.com 
 
Ping on this, Something just occured to me.  I was thinking (perhaps
erroneously) that this would go through the FCoE tree, but I don't see that
you've setup a tree yet vasu (and Rob's has been idle for 6 months).  Whats the
plan for this (and future) fcoe patchs.  Will you have a tree, or will we send
this through Christophs new scsi tree perhaps?

Neil

 
  spin_lock_irqsave(shost-host_lock, flags);
  rport-flags = ~FC_RPORT_DEVLOSS_PENDING;
  }
 
 
 
--
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

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

2014-06-03 Thread Neil Horman 
On Sat, May 31, 2014 at 08:37:57AM -0400, Neil Horman wrote:
 On Sat, May 31, 2014 at 05:13:11AM +, Eddie Wai wrote:
  
  
  On May 30, 2014, at 7:48 PM, Neil Horman nhor...@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]  [8169efec] dump_stack+0x19/0x1b
   [  290.489055]  [810ac7a9] __might_sleep+0x179/0x230
   [  290.489057]  [816a4ad5] mutex_lock_nested+0x55/0x520
   [  290.489061]  [a01b9905] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 
   [bnx2fc]
   [  290.489065]  [a0174c1a] fc_vport_id_lookup+0x3a/0xa0 
   [libfc]
   [  290.489068]  [a01b9a6c] bnx2fc_l2_rcv_thread+0x22c/0x4c0 
   [bnx2fc]
   [  290.489070]  [a01b9840] ? bnx2fc_vport_destroy+0x110/0x110 
   [bnx2fc]
   [  290.489073]  [8109e0cd] kthread+0xed/0x100
   [  290.489075]  [8109dfe0] ? insert_kthread_work+0x80/0x80
   [  290.489077]  [816b2fec] ret_from_fork+0x7c/0xb0
   [  290.489078]  [8109dfe0] ? 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-de...@open-fcoe.org
 http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
 
--
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

[PATCH] fc: ensure scan_work isn't active when freeing fc_rport

2014-05-30 Thread Neil Horman 
debugfs caught this:
WARNING: at lib/debugobjects.c:260 debug_print_object+0x83/0xa0()
ODEBUG: free active (active state 0) object type: work_struct
hint: fc_scsi_scan_rport+0x0/0xd0 [scsi_transport_fc]
 CPU: 1 PID: 184 Comm: kworker/1:1 Tainted: GW
--   3.10.0-123.el7.x86_64.debug #1
Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
Workqueue: fc_wq_5 fc_rport_final_delete [scsi_transport_fc]
Call Trace:
[8169efec] dump_stack+0x19/0x1b
[8106cbd1] warn_slowpath_common+0x61/0x80
[8106cc4c] warn_slowpath_fmt+0x5c/0x80
[8133e003] debug_print_object+0x83/0xa0
[a04e2f40] ? fc_parse_wwn+0x100/0x100

[8133f23b] debug_check_no_obj_freed+0x22b/0x270
[a04e127e] ? fc_rport_dev_release+0x1e/0x30
[811db3e9] kfree+0xd9/0x2d0
[a04e127e] fc_rport_dev_release+0x1e/0x30
[81428032] device_release+0x32/0xa0
[8132701e] kobject_release+0x7e/0x1b0
[81326ed8] kobject_put+0x28/0x60
[81428397] put_device+0x17/0x20
[a04e5025] fc_rport_final_delete+0x165/0x210
[810959b0] process_one_work+0x220/0x710
[81095944] ? process_one_work+0x1b4/0x710
[81095fbb] worker_thread+0x11b/0x3a0
[81095ea0] ? process_one_work+0x710/0x710
[8109e0cd] kthread+0xed/0x100
[8109dfe0] ? insert_kthread_work+0x80/0x80
[816b2fec] ret_from_fork+0x7c/0xb0
[8109dfe0] ? insert_kthread_work+0x80/0x80

Seems to be because the scan_work work_struct might be active when the housing
fc_rport struct gets freed.  Ensure that we cancel it prior to freeing the rport

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/scsi_transport_fc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 4628fd5..5bd552c 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2548,6 +2548,7 @@ fc_rport_final_delete(struct work_struct *work)
fc_flush_devloss(shost);
if (!cancel_delayed_work(rport-dev_loss_work))
fc_flush_devloss(shost);
+   cancel_work_sync(rport-scan_work);
spin_lock_irqsave(shost-host_lock, flags);
rport-flags = ~FC_RPORT_DEVLOSS_PENDING;
}
-- 
1.8.3.1

--
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

[PATCH] bnx2fc: Improve stats update mechanism

2014-05-30 Thread Neil Horman 
Recently had this warning reported:

[  290.489047] Call Trace:
[  290.489053]  [8169efec] dump_stack+0x19/0x1b
[  290.489055]  [810ac7a9] __might_sleep+0x179/0x230
[  290.489057]  [816a4ad5] mutex_lock_nested+0x55/0x520
[  290.489061]  [a01b9905] ? bnx2fc_l2_rcv_thread+0xc5/0x4c0 [bnx2fc]
[  290.489065]  [a0174c1a] fc_vport_id_lookup+0x3a/0xa0 [libfc]
[  290.489068]  [a01b9a6c] bnx2fc_l2_rcv_thread+0x22c/0x4c0 [bnx2fc]
[  290.489070]  [a01b9840] ? bnx2fc_vport_destroy+0x110/0x110 [bnx2fc]
[  290.489073]  [8109e0cd] kthread+0xed/0x100
[  290.489075]  [8109dfe0] ? insert_kthread_work+0x80/0x80
[  290.489077]  [816b2fec] ret_from_fork+0x7c/0xb0
[  290.489078]  [8109dfe0] ? 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 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);
 }
 
-- 
1.8.3.1

--
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

Re: [PATCH 4/8] libfc: Remove extra space in fc_exch_timer_cancel definition

2013-07-09 Thread Neil Horman 
On Tue, Jul 09, 2013 at 12:47:26PM -0700, Robert Love wrote:
 Simply remove an extra space that violates coding style.
 
 Signed-off-by: Robert Love robert.w.l...@intel.com
 Tested-by: Jack Morgan jack.mor...@intel.com
 Acked-by: Neil Horman nhor...@tuxdriver.com
 ---
  drivers/scsi/libfc/fc_exch.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
 index 8b928c6..e98ea6a 100644
 --- a/drivers/scsi/libfc/fc_exch.c
 +++ b/drivers/scsi/libfc/fc_exch.c
 @@ -337,7 +337,7 @@ static void fc_exch_release(struct fc_exch *ep)
   * fc_exch_timer_cancel() - cancel exch timer
   * @ep:  The exchange whose timer to be canceled
   */
 -static inline  void fc_exch_timer_cancel(struct fc_exch *ep)
 +static inline void fc_exch_timer_cancel(struct fc_exch *ep)
  {
   if (cancel_delayed_work(ep-timeout_work)) {
   FC_EXCH_DBG(ep, Exchange timer canceled\n);
 
 
Acked-by: Neil Horman nhor...@tuxdriver.com

--
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

Re: [PATCH 6/8] libfcoe: Fix meaningless log statement

2013-07-09 Thread Neil Horman 
On Tue, Jul 09, 2013 at 12:47:37PM -0700, Robert Love wrote:
 ctlr_dev was initialized to NULL, and never re-assigned. This
 caused the log statement to always report failure. This patch
 removes the unused variable and fixes the log statement to always
 report 'success', as that is what should be logged if the code
 reaches this point.
 
 Signed-off-by: Robert Love robert.w.l...@intel.com
 Tested-by: Jack Morgan jack.mor...@intel.com
 Acked-by: Neil Horman nhor...@tuxdriver.com
 ---
  drivers/scsi/fcoe/fcoe_transport.c |6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/scsi/fcoe/fcoe_transport.c 
 b/drivers/scsi/fcoe/fcoe_transport.c
 index bedd422..f1ae5ed 100644
 --- a/drivers/scsi/fcoe/fcoe_transport.c
 +++ b/drivers/scsi/fcoe/fcoe_transport.c
 @@ -707,7 +707,6 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
  {
   struct net_device *netdev = NULL;
   struct fcoe_transport *ft = NULL;
 - struct fcoe_ctlr_device *ctlr_dev = NULL;
   int rc = 0;
   int err;
  
 @@ -754,9 +753,8 @@ ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
   goto out_putdev;
   }
  
 - LIBFCOE_TRANSPORT_DBG(transport %s %s to create fcoe on %s.\n,
 -   ft-name, (ctlr_dev) ? succeeded : failed,
 -   netdev-name);
 + LIBFCOE_TRANSPORT_DBG(transport %s succeeded to create fcoe on %s.\n,
 +   ft-name, netdev-name);
  
  out_putdev:
   dev_put(netdev);
 
 

Acked-by: Neil Horman nhor...@tuxdriver.com
--
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

Re: [PATCH 5/8] libfc: Differentiate echange timer cancellation debug statements

2013-07-09 Thread Neil Horman 
On Tue, Jul 09, 2013 at 12:47:31PM -0700, Robert Love wrote:
 There are two debug statements with the same output string regarding
 echange timer cancellation. This patch simply changes the output of
 one string so that they can be differentiated.
 
 Signed-off-by: Robert Love robert.w.l...@intel.com
 Tested-by: Jack Morgan jack.mor...@intel.com
 Acked-by: Neil Horman nhor...@tuxdriver.com
 ---
  drivers/scsi/libfc/fc_exch.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
 index e98ea6a..5879929 100644
 --- a/drivers/scsi/libfc/fc_exch.c
 +++ b/drivers/scsi/libfc/fc_exch.c
 @@ -1567,7 +1567,7 @@ static void fc_exch_abts_resp(struct fc_exch *ep, 
 struct fc_frame *fp)
   fc_exch_rctl_name(fh-fh_r_ctl));
  
   if (cancel_delayed_work_sync(ep-timeout_work)) {
 - FC_EXCH_DBG(ep, Exchange timer canceled\n);
 + FC_EXCH_DBG(ep, Exchange timer canceled due to ABTS 
 response\n);
   fc_exch_release(ep);/* release from pending timer hold */
   }
  
 
 

Acked-by: Neil Horman nhor...@tuxdriver.com
--
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

[PATCH] libcxgbi: supress warning when we request to much space from kmalloc

2013-04-23 Thread Neil Horman 
cxgbi_alloc_big_mem allocates large chunks of memory, and can occasionally
request amounts from kmalloc that exceed the allocators capacity.  This
typically leads to a stack trace from the zoned buddy allocator in the message
log.  But if kmalloc fails, cxgbi_alloc_big_mem backs off and uses vmalloc
instead.  Given that, and the fact that the two calls sites have their own error
messages if both kmalloc and vmalloc fail, I think the stack trace printing
isn't really needed.

Modify the call to kmalloc to pass __GFP_NOWARN in as well, so that internal
kmalloc warnings are suppressed.

Signed-off-by: Neil Horman nhor...@tuxdriver.com
Reported-by: Honggang LI ho...@redhat.com
CC: James E.J. Bottomley jbottom...@parallels.com
CC: linux-ker...@vger.kernel.org
---
 drivers/scsi/cxgbi/libcxgbi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
index 80fa99b..3daf996 100644
--- a/drivers/scsi/cxgbi/libcxgbi.h
+++ b/drivers/scsi/cxgbi/libcxgbi.h
@@ -658,7 +658,7 @@ static inline u32 cxgbi_tag_nonrsvd_bits(struct 
cxgbi_tag_format *tformat,
 static inline void *cxgbi_alloc_big_mem(unsigned int size,
gfp_t gfp)
 {
-   void *p = kmalloc(size, gfp);
+   void *p = kmalloc(size, gfp | __GFP_NOWARN);
if (!p)
p = vmalloc(size);
if (p)
-- 
1.8.1.4

--
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

Re: [PATCH v2] libfcoe: Save some memory and optimize name lookups

2012-12-17 Thread Neil Horman 
On Fri, Dec 14, 2012 at 02:02:18PM -0800, Robert Love wrote:
 Instead of creating a structure with an enum and a pointer
 to a string, simply allocate an array of strings and use
 the enum values for the indicies.
 
 This means that we do not need to iterate through the list
 of entries when looking up a string name by its enum key.
 
 This will also help with a latter patch that will add
 more fcoe_sysfs attributes that will also use the
 fcoe_enum_name_search macro. One attribute will also do
 a reverse lookup which requires less code when the
 enum-to-string mappings are organized as this patch makes
 them to be.
 
 Signed-off-by: Robert Love robert.w.l...@intel.com
 Acked-by: Neil Horman nhor...@tuxdriver.com
 ---
  drivers/scsi/fcoe/fcoe_sysfs.c |   44 
 +++-
  1 file changed, 16 insertions(+), 28 deletions(-)
 
 diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
 index 5e75168..e6fce28 100644
 --- a/drivers/scsi/fcoe/fcoe_sysfs.c
 +++ b/drivers/scsi/fcoe/fcoe_sysfs.c
 @@ -210,25 +210,23 @@ static ssize_t show_fcoe_fcf_device_##field(struct 
 device *dev, \
  #define fcoe_enum_name_search(title, table_type, table)  
 \
  static const char *get_fcoe_##title##_name(enum table_type table_key)
 \
  {\
 - int i;  \
 - char *name = NULL;  \
 - \
 - for (i = 0; i  ARRAY_SIZE(table); i++) {   \
 - if (table[i].value == table_key) {  \
 - name = table[i].name;   \
 - break;  \
 - }   \
 - }   \
 - return name;\
 + if (table_key  0 || table_key = ARRAY_SIZE(table))\
 + return NULL;\
 + return table[table_key];\
  }
  
 -static struct {
 - enum fcf_state value;
 - char   *name;
 -} fcf_state_names[] = {
 - { FCOE_FCF_STATE_UNKNOWN,  Unknown },
 - { FCOE_FCF_STATE_DISCONNECTED, Disconnected },
 - { FCOE_FCF_STATE_CONNECTED,Connected },
 +static char *fip_conn_type_names[] = {
 + [ FIP_CONN_TYPE_UNKNOWN ] = Unknown,
 + [ FIP_CONN_TYPE_FABRIC ]  = Fabric,
 + [ FIP_CONN_TYPE_VN2VN ]   = VN2VN,
 +};
 +fcoe_enum_name_search(ctlr_mode, fip_conn_type, fip_conn_type_names)
 +#define FCOE_CTLR_MODE_MAX_NAMELEN 50
 +
 +static char *fcf_state_names[] = {
 + [ FCOE_FCF_STATE_UNKNOWN ]  = Unknown,
 + [ FCOE_FCF_STATE_DISCONNECTED ] = Disconnected,
 + [ FCOE_FCF_STATE_CONNECTED ]= Connected,
  };
  fcoe_enum_name_search(fcf_state, fcf_state, fcf_state_names)
  #define FCOE_FCF_STATE_MAX_NAMELEN 50
 @@ -246,17 +244,7 @@ static ssize_t show_fcf_state(struct device *dev,
  }
  static FCOE_DEVICE_ATTR(fcf, state, S_IRUGO, show_fcf_state, NULL);
  
 -static struct {
 - enum fip_conn_type value;
 - char   *name;
 -} fip_conn_type_names[] = {
 - { FIP_CONN_TYPE_UNKNOWN, Unknown },
 - { FIP_CONN_TYPE_FABRIC, Fabric },
 - { FIP_CONN_TYPE_VN2VN, VN2VN },
 -};
 -fcoe_enum_name_search(ctlr_mode, fip_conn_type, fip_conn_type_names)
 -#define FCOE_CTLR_MODE_MAX_NAMELEN 50
 -
 +#define FCOE_MAX_MODENAME_LEN 20
  static ssize_t show_ctlr_mode(struct device *dev,
 struct device_attribute *attr,
 char *buf)
 
 
Acked-by: Neil Horman nhor...@tuxdriver.com

--
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

Re: [Open-FCoE] [RFC PATCH v4 0/5] Add new fcoe_sysfs based control interfaces to libfcoe, bnx2fc and fcoe

2012-10-10 Thread Neil Horman 
On Tue, Oct 09, 2012 at 03:18:50PM -0700, Robert Love wrote:
 v4:
 
 @Neil:
   Policy is now:
 
   'mode' attribute:
  input: Fabric VN2VN (case insensitive)
  output: Fabric VN2VN
 
   'enabled' attribute:
  input: 1 0
  output: 1 0
 
Thanks, for the series
Acked-by: Neil Horman nhor...@tuxdriver.com

 @Mark:
   Initial patch now optimizes enum-to-string memory usage and
   string retreival
 
 --
 
 v3:
 
 This series applies to the v3.6 kernel.
 
 @Bart: Fixed bus_create_file - bus_attrs
Removed sscanf with non-NULL buffer, only checking for '1' or '0' now
Removed unnecessary whitespace change
 
 @Bhanu: Incorporated check in _bnx2fc_create (thanks for the code)
 
 Additional changes: Added check for 'enabled' in reset in fcoe.c
   Made mode strncmp case insensitive so user can
   # echo vn2vn  /sys/.../ctlr_0/mode # or
   # echo VN2VN  /sys/.../ctlr_0/mode # or even
   # echo FaBRiC  /sys/.../ctlr_0/mode
 
 --
 
 v2:
 
 The following series adds /sys/bus/fcoe based control interfaces to
 libfcoe. A fcoe_sysfs infrastructure was added to the kernel a few
 cycles ago, this series builds on that work. The patches deprecate
 the old create, vn2vn_create, destroy, enable and disable interfaces
 that exist in /sys/module/libfcoe/parameters/.
 
 Another goal of this series is to change the initialization sequence for
 a FCoE device. The result of this series is that interfaces created using
 libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the following
 starting steps-
 
 1) Create/alloc the FCoE Controller
- Allocate kernel memory and create per-instance sysfs devices
- The FCoE Controller is created disabled, no discovery or login
  until it is enabled.
 
# echo eth3.172-fcoe  /sys/bus/fcoe/ctlr_create
 
 2) Configure the FCoE Controller
- Change mode, set ddp_min (future), set dcb_required (future), etc...
 
# echo 2  /sys/bus/fcoe/ctlr_0/mode #sets mode to VN2VN
 
 3) Enable the FCoE Controller
- Begins discovery and login in 'Fabric' mode. or
- Begins FIP FCID claim process, discovery and login in 'VN2VN' mode
 
 4) Destroy the FCoE Controller
- Logout and free all memory
 
# echo eth3.172-fcoe  /sys/bus/fcoe/ctlr_destroy
 
 This series converts both fcoe.ko and bnx2fc.ko to use the new
 fcoe_sysfs based interfaces. The legacy interfaces can remain in kernel
 for a kernel cycle or two before being removed.
 
 I'm looking for feedback on the use of /sys/bus/fcoe/ctlr_create and
 /sys/bus/fcoe/ctlr_destroy and that those interfaces take an ifname.
 I modeled it after the bonding interface, but I'm not sure if that's
 acceptible.
 
 Incorpoated v1 feedback:
 
 @Chris:
 
 I spent a lot of time looking into FCF selection.
 
 I implemented a POC series where I made the 'enabled' attribute
 of a fcoe_fcf_device (i.e. /sys/bus/fcoe/devices/fcf_X) writable. The
 fcoe_ctlr_device (i.e. /sys/bus/fcoe/devices/ctlr_X) has a
 'selection_strategy' attribute that would allow the user to toggle between
 AUTO (current kernel selection algorithm) and USER (user writes to FCF's
 'selection' attribute).
 
 I also wrote a little libudev based application that listened for 
 fcoe_fcf_device
 sysfs add/remove events. My plan was to have fcoemon monitor the discovery
 of FCFs and then have it write to the 'selected' attribute of the FCF the user
 had chosen.
 
 Working on this series convinced me that any FCF selection needs further
 consideration. First of all, it's fairly complicated to update the fcoe_ctlr.c
 functional FIP code to handle FCF/fabric selection. Some questions that arise:
 
 What triggers FLOGI/LOGO? We would now have link, enabled/disabled, selection
 strategy and FCF selection to consider.
 
 Can a new FCF be selected when one is already selected and an FLOGI has 
 occurred?
 Can a FCF be de-selected when the FLOGI has been sent?
 
 Maybe we can only change things when disabled, that probably makes the most 
 sense..
 
 When does discovery happen? When the ctlr is created (no opportunity for
 mode/selection strategy to be set)? When the mode is changed (same problem)?
 What about when the cltr is enabled (but that's when we were going to FLOGI)?
 
 This isn't a complete list of considerations, just what came to mind when 
 writing
 this. Anyway, the policies started to get complicated, or maybe my lack of 
 policies
 made the POC implementation more complicated. Either way it made me think 
 about
 use cases and how valuable FCF/fabric selection is.
 
 After further consideration I think that most of the access decisions, when
 connecting to a FC fabric, should be done by the fabric services. I'm not sure
 the endstations should be whitelisting or blacklisting FCFs or even making
 decisions about which ones to use when they're already prioritized amongst 
 themselves
 (on the same fabric). I think it might

Re: Request for improved commit tracking between fcoe and scsi trees

2012-10-08 Thread Neil Horman 
On Sun, Oct 07, 2012 at 11:59:29AM +0100, James Bottomley wrote:
 On Wed, 2012-10-03 at 15:23 -0400, Neil Horman wrote:
  James, Robert-
  I've been doing lots of backports of FCoE code to the RHEL tree these
  last few months, and I've noticed something fairly irritating, and I was
  wondering if you two could help me out with it (in fact you two are the 
  only two
  which can).  I noticed that commits which are accepted into the FCoE tree 
  that
  get passed upstream through the scsi tree have their commit hashes altered. 
   I
  can't find any examples currently, due to the fact that you, Robert, have
  recently re-cloned your git tree at open-fcoe.org, so all this nastiness has
  been covered up currently, but if things don't change, this issue will 
  quickly
  resurface.
  
  Regardless, This makes it _really_ difficult to track a given patchs' 
  traversal 
  between trees upstream, and makes my life as a distro subsystem maintainer 
  fairly
  painful.  Normally I would just live with it, but I can't see any reason 
  why it
  should be this way, given that git can easily prevent this with a pull.  
  James,
  Robert, could you two please work out a way to provide commit hash 
  consistency
  between your trees?  It would make mine (and I'm sure many other people's)
  lives, much easier.
 
 I'm reluctant to commit to any tracking process that relies on stable
 commit ids simply because they're illusory in git:  if we find a bug in
 a commit (or even worse a bisection failure) in a tree, we fix it there,
 which causes the commit id to change.
 
 The way I do this type of tracking is via the Subject: line ... why
 can't you use that?
 
Because git workflows are rooted in the notion that (illusory or not),
commits are immutable and stable.  Like it or not, its how changes (generally
speaking) get tracked.  Every time you rewrite history, that gets messed up.
And yes, I (and any one else) pulling changes from the scsi tree could track
things based on Subject line, but thats got its own problems, as multiple trees
run the risk of of having the same trivial subject line, which is far more
likely to occur than a sha collision.  It would also require a customization of
my workflow specifically for the scsi tree that differs from any other tree that
I follow.

I think in short, I would far prefer to see a pull/merge strategy from your
downtream contributing trees, with myself handling the risk of having to do a
whole series fixup should you need to fix something during a rewrite in the scsi
tree.  I'd gladly accept that risk in exchange for the ability to handle your
tree like I do others.  It seems from your previous note that you're will to go
that route, and I certainly appreciate that.

Thanks!
Neil

 James
 
 
 
--
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

Re: [Open-FCoE] [RFC PATCH v3 2/4] libfcoe, fcoe, bnx2fc: Add new fcoe control interface

2012-10-08 Thread Neil Horman 
On Fri, Oct 05, 2012 at 09:54:32AM -0700, Robert Love wrote:
 This patch does a few things.
 
 1) Makes /sys/bus/fcoe/ctlr_{create,destroy} interfaces.
These interfaces take an ifname and will either
create an FCoE Controller or destroy an FCoE
Controller depending on which file is written to.
 
The new FCoE Controller will start in a DISABLED
state and will not do discovery or login until it
is ENABLED. This pause will allow us to configure
the FCoE Controller before enabling it.
 
 2) Makes the 'mode' attribute of a fcoe_ctlr_device
writale. This allows the user to configure the mode
in which the FCoE Controller will start in when it
is ENABLED.
 
Possible modes are 'Fabric', or 'VN2VN'.
 
The default mode for a fcoe_ctlr{,_device} is 'Fabric'.
Drivers must implement the set_fcoe_ctlr_mode routine
to support this feature.
 
libfcoe offers an exported routine to set a FCoE
Controller's mode. The mode can only be changed
when the FCoE Controller is DISABLED.
 
This patch also removes the get_fcoe_ctlr_mode pointer
in the fcoe_sysfs function template, the code in
fcoe_ctlr.c to get the mode and the assignment of
the fcoe_sysfs function pointer to the fcoe_ctlr.c
implementation (in fcoe and bnx2fc). fcoe_sysfs can
return that value for the mode without consulting the
LLD.
 
 3) Make a 'enabled' attribute of a fcoe_ctlr_device. On a
read, fcoe_sysfs will return the attribute's value. On
a write, fcoe_sysfs will call the LLD (if there is a
callback) to notifiy that the enalbed state has changed.
 
 This patch maintains the old FCoE control interfaces as
 module parameters, but it adds comments pointing out that
 the old interfaces are deprecated.
 
 Signed-off-by: Robert Love robert.w.l...@intel.com
 ---
  Documentation/ABI/testing/sysfs-bus-fcoe |   42 +++
  drivers/scsi/bnx2fc/bnx2fc_fcoe.c|1 
  drivers/scsi/fcoe/fcoe.c |1 
  drivers/scsi/fcoe/fcoe_sysfs.c   |  186 
 +++---
  drivers/scsi/fcoe/fcoe_transport.c   |  104 +
  include/scsi/fcoe_sysfs.h|   11 ++
  include/scsi/libfcoe.h   |   16 ++-
  7 files changed, 338 insertions(+), 23 deletions(-)
 
 diff --git a/Documentation/ABI/testing/sysfs-bus-fcoe 
 b/Documentation/ABI/testing/sysfs-bus-fcoe
 index 469d09c..a57bf37 100644
 --- a/Documentation/ABI/testing/sysfs-bus-fcoe
 +++ b/Documentation/ABI/testing/sysfs-bus-fcoe
 @@ -1,14 +1,54 @@
 +What:/sys/bus/fcoe/
 +Date:August 2012
 +KernelVersion:   TBD
 +Contact: Robert Love robert.w.l...@intel.com, de...@open-fcoe.org
 +Description: The FCoE bus. Attributes in this directory are control 
 interfaces.
 +Attributes:
 +
 + ctlr_create: 'FCoE Controller' instance creation interface. Writing an
 +  ifname to this file will allocate and populate sysfs 
 with a
 +  fcoe_ctlr_device (ctlr_X). The user can then configure any
 +  per-port settings and finally write to the 
 fcoe_ctlr_device's
 +  'start' attribute to begin the kernel's discovery and login
 +  process.
 +
 + ctlr_destroy: 'FCoE Controller' instance removal interface. Writing a
 +fcoe_ctlr_device's sysfs name to this file will log the
 +fcoe_ctlr_device out of the fabric or otherwise connected
 +FCoE devices. It will also free all kernel memory 
 allocated
 +for this fcoe_ctlr_device and any structures associated
 +with it, this includes the scsi_host.
 +
  What:/sys/bus/fcoe/ctlr_X
  Date:March 2012
  KernelVersion:   TBD
  Contact: Robert Love robert.w.l...@intel.com, de...@open-fcoe.org
 -Description: 'FCoE Controller' instances on the fcoe bus
 +Description: 'FCoE Controller' instances on the fcoe bus.
 +
 + The FCoE Controller now has a three stage creation process.
 + 1) Write interface name to ctlr_create 2) Configure the FCoE
 + Controller (ctlr_X) 3) Enable the FCoE Controller to begin
 + discovery and login. The FCoE Controller is destroyed by
 + writing it's name, i.e. ctlr_X to the ctlr_delete file.
 +
  Attributes:
  
   fcf_dev_loss_tmo: Device loss timeout peroid (see below). Changing
 this value will change the dev_loss_tmo for all
 FCFs discovered by this controller.
  
 + mode: Display or change the FCoE Controller's mode. Possible
 +   modes are 'Fabric' and 'VN2VN'. If a FCoE Controller
 +   is started in 'Fabric' mode then FIP FCF discovery is
 +   initiated and ultimately a fabric login is attempted.
 +   If a FCoE Controller is started

Re: [Open-FCoE] [PATCH] scsi_transport_fc: Add sysfs entry for HBA API management library

2012-10-04 Thread Neil Horman 
On Thu, Oct 04, 2012 at 09:31:08PM +, Love, Robert W wrote:
 On 10/4/2012 12:08 AM, Hannes Reinecke wrote:
  On 10/03/2012 10:12 PM, Robert Love wrote:
  The user space HBA API vendor libraries need to know which HBA/CNAs
  hosts to manage. Currently, libhbalinux is used to manage a few drivers
  that use libfcoe and libfc. Right now libhbalinux keys off of the
  string  over  in the FC Host's symbolic_name attribute to determine
  if it should manage a given host. fnic, bnx2fc and fcoe/netdev based
  hosts all use  over  in their symboic_names and that is currently
  what libhbalinux looks for to determine if it should manage hosts
  created by those drivers.
 
  Clearly using  over  in the symbolic_name isn't descriptive
  and it is an awkward way to determine whether libhbalinux should
  manage a host. Also, drivers may wish to use more descriptive
  and accurate symbolic_names because these strings are displayed
  in fabric management applications; forcing them to use  over 
  in their symbolic_name is unnecessarily restrictive.
 
  This patch adds a read-only attribute to the FC Host that will expose
  which management library should be used to manage it.
 
  This attribute will not be present in sysfs for drivers that
  do no implement the .show_hbaapi_lib routine.
 
  Signed-off-by: Robert Love robert.w.l...@intel.com
  Tested-by: Ross Brattain ross.b.bratt...@intel.com
  ---
drivers/scsi/bnx2fc/bnx2fc_fcoe.c |4 
drivers/scsi/fcoe/fcoe.c  |4 
drivers/scsi/fnic/fnic_main.c |4 
drivers/scsi/scsi_transport_fc.c  |5 -
include/scsi/libfc.h  |2 ++
include/scsi/scsi_transport_fc.h  |4 
6 files changed, 22 insertions(+), 1 deletion(-)
 
  diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
  b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
  index ae1cb76..97f60d8 100644
  --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
  +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
  @@ -731,6 +731,9 @@ static int bnx2fc_shost_config(struct fc_lport 
  *lport, struct device *dev)
BNX2FC_NAME, BNX2FC_VERSION,
interface-netdev-name);
 
  +strncpy(fc_host_hbaapi_library(lport-host), FC_LIBHBALINUX_NAME,
  +FC_SYMBOLIC_NAME_SIZE);
  +
return 0;
}
 
  @@ -2583,6 +2586,7 @@ static struct fc_function_template 
  bnx2fc_transport_function = {
.get_host_port_state = fc_get_host_port_state,
.show_host_port_state = 1,
.show_host_symbolic_name = 1,
  +.show_host_hbaapi_library = 1,
 
.dd_fcrport_size = (sizeof(struct fc_rport_libfc_priv) +
sizeof(struct bnx2fc_rport)),
  diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
  index 078d262..812876f 100644
  --- a/drivers/scsi/fcoe/fcoe.c
  +++ b/drivers/scsi/fcoe/fcoe.c
  @@ -201,6 +201,7 @@ static struct fc_function_template 
  fcoe_nport_fc_functions = {
.get_host_port_state = fc_get_host_port_state,
.show_host_port_state = 1,
.show_host_symbolic_name = 1,
  +.show_host_hbaapi_library = 1,
 
.dd_fcrport_size = sizeof(struct fc_rport_libfc_priv),
.show_rport_maxframe_size = 1,
  @@ -755,6 +756,9 @@ static int fcoe_shost_config(struct fc_lport 
  *lport, struct device *dev)
 %s v%s over %s, FCOE_NAME, FCOE_VERSION,
 fcoe_netdev(lport)-name);
 
  +strncpy(fc_host_hbaapi_library(lport-host), FC_LIBHBALINUX_NAME,
  +FC_SYMBOLIC_NAME_SIZE);
  +
return 0;
}
 
  diff --git a/drivers/scsi/fnic/fnic_main.c 
  b/drivers/scsi/fnic/fnic_main.c
  index fc98eb6..783bd3d 100644
  --- a/drivers/scsi/fnic/fnic_main.c
  +++ b/drivers/scsi/fnic/fnic_main.c
  @@ -138,6 +138,7 @@ static struct fc_function_template 
  fnic_fc_functions = {
.get_host_port_state = fc_get_host_port_state,
.show_host_port_state = 1,
.show_host_symbolic_name = 1,
  +.show_host_hbaapi_library = 1,
.show_rport_maxframe_size = 1,
.show_rport_supported_classes = 1,
.show_host_fabric_name = 1,
  @@ -704,6 +705,9 @@ static int __devinit fnic_probe(struct pci_dev 
  *pdev,
sprintf(fc_host_symbolic_name(lp-host),
DRV_NAME  v DRV_VERSION  over %s, fnic-name);
 
  +strncpy(fc_host_hbaapi_library(lp-host), FC_LIBHBALINUX_NAME,
  +FC_SYMBOLIC_NAME_SIZE);
  +
spin_lock_irqsave(fnic_list_lock, flags);
list_add_tail(fnic-list, fnic_list);
spin_unlock_irqrestore(fnic_list_lock, flags);
  diff --git a/drivers/scsi/scsi_transport_fc.c 
  b/drivers/scsi/scsi_transport_fc.c
  index e894ca7..ed19b42 100644
  --- a/drivers/scsi/scsi_transport_fc.c
  +++ b/drivers/scsi/scsi_transport_fc.c
  @@ -313,7 +313,7 @@ static void fc_scsi_scan_rport(struct work_struct 
  *work);
#define FC_STARGET_NUM_ATTRS 3
#define FC_RPORT_NUM_ATTRS10
#define FC_VPORT_NUM_ATTRS9
  -#define FC_HOST_NUM_ATTRS29
  +#define FC_HOST_NUM_ATTRS30
 
struct