On Thu, Jun 21, 2012 at 10:13:17AM -0700, Vasu Dev wrote:
> On Wed, 2012-06-20 at 08:48 -0400, Neil Horman wrote:
> > Noticed that we can shuffle the code around in fcoe_percpu_receive_thread a
> > bit
> > and avoid taking the fcoe_rx_list lock twice per iteration. This should
> > improve
> > throughput somewhat. With this change we take the lock, and check for new
> > frames in a single critical section. Only if the list is empty do we drop
> > the
> > lock and re-acquire it after being signaled to wake up.
> >
> > Signed-off-by: Neil Horman <nhor...@tuxdriver.com>
> > CC: Robert Love <robert.w.l...@intel.com>
> > ---
> > drivers/scsi/fcoe/fcoe.c | 18 ++++++++++--------
> > 1 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index 0b48c7d..082951f 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1820,19 +1820,21 @@ static int fcoe_percpu_receive_thread(void *arg)
> >
> > spin_lock_bh(&p->fcoe_rx_list.lock);
> > skb_queue_splice_init(&p->fcoe_rx_list, &tmp);
> > - spin_unlock_bh(&p->fcoe_rx_list.lock);
> > -
> > - while ((skb = __skb_dequeue(&tmp)) != NULL)
> > - fcoe_recv_frame(skb);
> >
> > - spin_lock_bh(&p->fcoe_rx_list.lock);
> > - if (!skb_queue_len(&p->fcoe_rx_list)) {
> > + while (!skb_queue_len(&tmp) && !kthread_should_stop()) {
>
> If skb_queue_len check is done on locked fcoe_rx_list then
> skb_queue_splice_init() can be moved outside loop below with its only
> one instance calling instead having twice. Just very minor pick along
> this cleanup and anyway it should get called once on each thread wakeup
> with you patch.
>
Good call, you're right. Although it gets a little tricky because you need to
hold the list lock to prevent a race with the softirq waking you up right before
you go to sleep. I can replace the additional lock/splice_init with a goto
however, just to make it a bit mroe consice. That also lets me eliminate the
second kthread_should_stop check. I'll repost with that change shortly.
Thanks!
> Good lock cleanup but I'm not sure though if we could notice measurable
> throughput difference since rx thread and lists are per cpu. However it
> should help in case of contention with soft-irq context as you also
> mentioned in patch description.
>
Yeah, this isn't a major performance boost, its mostly just cleanup. It will
help in the pessimal case of a single cpu system though I think.
Neil
> Thanks
> Vasu
>
> > set_current_state(TASK_INTERRUPTIBLE);
> > spin_unlock_bh(&p->fcoe_rx_list.lock);
> > schedule();
> > set_current_state(TASK_RUNNING);
> > - } else
> > - spin_unlock_bh(&p->fcoe_rx_list.lock);
> > + spin_lock_bh(&p->fcoe_rx_list.lock);
> > + skb_queue_splice_init(&p->fcoe_rx_list, &tmp);
> > + }
> > +
> > + spin_unlock_bh(&p->fcoe_rx_list.lock);
> > +
> > + while ((skb = __skb_dequeue(&tmp)) != NULL)
> > + fcoe_recv_frame(skb);
> > +
> > }
> > return 0;
> > }
>
>
>
_______________________________________________
devel mailing list
devel@open-fcoe.org
https://lists.open-fcoe.org/mailman/listinfo/devel
