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" <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] [<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-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
