RE: [PATCH 5/8] fcoe: reduces lock cost when adding a new skb to fcoe_pending_queue

James Bottomley
2009-05-06T20:43:09Z

On Wed, 2009-05-06 at 13:28 -0700, Love, Robert W wrote:
> James Bottomley wrote:
> > On Wed, 2009-05-06 at 10:52 -0700, Robert Love wrote:
> >> From: Vasu Dev <vasu.dev@intel.com>
> >>
> >> Currently fcoe_pending_queue.lock held twice for every new skb
> >> adding to this queue when already least one pkt is pending in this
> >> queue and that is not uncommon once skb pkts starts getting queued
> >> here upon fcoe_start_io => dev_queue_xmit failure.
> >>
> >> This patch moves most fcoe_pending_queue logic to
> >> fcoe_check_wait_queue function, this new logic grabs
> >> fcoe_pending_queue.lock only once to add a new skb instead twice as
> >> used to be.
> >>
> >> I think after this patch call flow around fcoe_check_wait_queue
> >> calling in fcoe_xmit is bit simplified with modified
> >> fcoe_check_wait_queue function taking care of adding and
> >> removing pending skb in one function.
> >
> > This isn't really a -rc4 bug fix, is it? It reads a lot more like a
> > feature enhancement that was given a bug like description.
> >
> You're right- it is an enhancement and doesn't fix a bug (that I'm aware
> of). I've pulled it out of the set (locally) and re-tested; there are
> no dependency issues with removing it from this update.
>
> What about the "6/8 removes fcoe_watchdog" patch? I think your reasoning
> could apply to that one as well. It's replacing the global fcoe watchdog
> with a per-interface one, which could be clasified as an enhancement
> as well. (I've removed this patch also (locally) and tested, so if you
> think it should be pushed later, removing it won't cause any issues.)--

That was next in my sights ... I was just waiting to see whether you
noticed.

James