[v4,0/10] bql: Byte Queue Limits

On Tue, Nov 29, 2011 at 10:31:03AM -0800, Tom Herbert wrote:
> On Tue, Nov 29, 2011 at 9:47 AM, David Miller <davem@davemloft.net> wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 29 Nov 2011 17:46:24 +0100
> >
> >> I did sucessful tests with tg3 (I'll provide the patch for bnx2 shortly)
> >>
> >> Some details probably can be polished, but I believe your v4 is ready
> >> for inclusion.
> >>
> >> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> >
> > Agreed, all applied to net-next, thanks!
> >
> > Tom, please keep an eye out for regression or suggestion reports.
> >
> 
> Will do.  I am well aware of how invasive this is in the data path ;-)
>  I'll add a doc describing BQL also.
> 
> By the way, the way to disable BQL at runtime is the 'echo max >
> /sys/class/net/eth<n>/queues/tx-<m>/byte_queue_limits/limit_min

One "regression" is it is now not possible to disable BQL at compile time,
because CONFIG_BQL can't be set to "n" via usual ways.

Description and patch below. Thanks.


---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Thu, 1 Dec 2011 20:36:06 +0400
Subject: [PATCH] net: Allow users to set CONFIG_BQL=n

Commit 114cf580 (bql: Byte queue limits) added new config option without
description, which means neither `make oldconfig` nor `make menuconfig`
ask for it -- the option is simply set to default y automatically.

Make the option actually configurable via adding stub description.

Cc: Tom Herbert <therbert@google.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 net/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Thu, 1 Dec 2011 20:50:18 +0400

> One "regression" is it is now not possible to disable BQL at compile time,
> because CONFIG_BQL can't be set to "n" via usual ways.
> 
> Description and patch below. Thanks.

It's intentional, and your patch will not be applied.

The Kconfig entry and option is merely for expressing internal dependencies,
not for providing a way to disable the code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 01, 2011 at 01:00:45PM -0500, David Miller wrote:
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Thu, 1 Dec 2011 20:50:18 +0400
> 
> > One "regression" is it is now not possible to disable BQL at compile time,
> > because CONFIG_BQL can't be set to "n" via usual ways.
> > 
> > Description and patch below. Thanks.
> 
> It's intentional, and your patch will not be applied.
> 
> The Kconfig entry and option is merely for expressing internal dependencies,
> not for providing a way to disable the code.

I'm maybe wrong somewhere - sorry then, but why there is e.g.

     static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
                                            unsigned int bytes)
     {
    +#ifdef CONFIG_BQL
    +       dql_queued(&dev_queue->dql, bytes);
    +       if (unlikely(dql_avail(&dev_queue->dql) < 0)) {
    +               set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);
    +               if (unlikely(dql_avail(&dev_queue->dql) >= 0))
    +                       clear_bit(__QUEUE_STATE_STACK_XOFF,
    +                           &dev_queue->state);
    +       }
    +#endif
     }


and that netdev_tx_sent_queue() is called on every xmit.

I wanted to save cycles on my small/slow hardware and compile BQL out in
case I know the system is not going to use it. I'm netdev newcomer, so
sorry if I ask naive questions, but what's wrong it?


Thanks,
Kirill
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Le vendredi 02 décembre 2011 à 15:22 +0400, Kirill Smelkov a écrit :
> On Thu, Dec 01, 2011 at 01:00:45PM -0500, David Miller wrote:
> > From: Kirill Smelkov <kirr@mns.spb.ru>
> > Date: Thu, 1 Dec 2011 20:50:18 +0400
> > 
> > > One "regression" is it is now not possible to disable BQL at compile time,
> > > because CONFIG_BQL can't be set to "n" via usual ways.
> > > 
> > > Description and patch below. Thanks.
> > 
> > It's intentional, and your patch will not be applied.
> > 
> > The Kconfig entry and option is merely for expressing internal dependencies,
> > not for providing a way to disable the code.
> 
> I'm maybe wrong somewhere - sorry then, but why there is e.g.
> 
>      static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
>                                             unsigned int bytes)
>      {
>     +#ifdef CONFIG_BQL
>     +       dql_queued(&dev_queue->dql, bytes);
>     +       if (unlikely(dql_avail(&dev_queue->dql) < 0)) {
>     +               set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);
>     +               if (unlikely(dql_avail(&dev_queue->dql) >= 0))
>     +                       clear_bit(__QUEUE_STATE_STACK_XOFF,
>     +                           &dev_queue->state);
>     +       }
>     +#endif
>      }
> 
> 
> and that netdev_tx_sent_queue() is called on every xmit.
> 
> I wanted to save cycles on my small/slow hardware and compile BQL out in
> case I know the system is not going to use it. I'm netdev newcomer, so
> sorry if I ask naive questions, but what's wrong it?
> 

Its something like 4 or 5 instructions (granted your NIC is BQL
enabled). Really nothing compared to thousand of instructions per packet
spent in the stack and driver.

And BQL will benefit especially on your small/slow hardware, even if you
dont believe so or know yet.

BQL is probably the major new netdev functionality of the year.

Of course, you are free to patch your own kernel if you desperatly need
to save a few cycles per packet.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Dec 02, 2011 at 12:57:51PM +0100, Eric Dumazet wrote:
> Le vendredi 02 décembre 2011 à 15:22 +0400, Kirill Smelkov a écrit :
> > On Thu, Dec 01, 2011 at 01:00:45PM -0500, David Miller wrote:
> > > From: Kirill Smelkov <kirr@mns.spb.ru>
> > > Date: Thu, 1 Dec 2011 20:50:18 +0400
> > > 
> > > > One "regression" is it is now not possible to disable BQL at compile time,
> > > > because CONFIG_BQL can't be set to "n" via usual ways.
> > > > 
> > > > Description and patch below. Thanks.
> > > 
> > > It's intentional, and your patch will not be applied.
> > > 
> > > The Kconfig entry and option is merely for expressing internal dependencies,
> > > not for providing a way to disable the code.
> > 
> > I'm maybe wrong somewhere - sorry then, but why there is e.g.
> > 
> >      static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
> >                                             unsigned int bytes)
> >      {
> >     +#ifdef CONFIG_BQL
> >     +       dql_queued(&dev_queue->dql, bytes);
> >     +       if (unlikely(dql_avail(&dev_queue->dql) < 0)) {
> >     +               set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);
> >     +               if (unlikely(dql_avail(&dev_queue->dql) >= 0))
> >     +                       clear_bit(__QUEUE_STATE_STACK_XOFF,
> >     +                           &dev_queue->state);
> >     +       }
> >     +#endif
> >      }
> > 
> > 
> > and that netdev_tx_sent_queue() is called on every xmit.
> > 
> > I wanted to save cycles on my small/slow hardware and compile BQL out in
> > case I know the system is not going to use it. I'm netdev newcomer, so
> > sorry if I ask naive questions, but what's wrong it?
> > 
> 
> Its something like 4 or 5 instructions (granted your NIC is BQL
> enabled). Really nothing compared to thousand of instructions per packet
> spent in the stack and driver.
> 
> And BQL will benefit especially on your small/slow hardware, even if you
> dont believe so or know yet.
> 
> BQL is probably the major new netdev functionality of the year.
> 
> Of course, you are free to patch your own kernel if you desperatly need
> to save a few cycles per packet.

Thanks for describing this Eric. I'll try BQL and see how it goes as you
suggest. Please note that the confusion here could be avoided if there
was a tiny bit of documentation.


Thanks again,
Kirill
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html