[PATCH net 0/3] net: bql: better deal with GSO

[PATCH net 0/3] net: bql: better deal with GSO

[Eric Dumazet]

While BQL bulk dequeue works well for TSO packets, it is
not very efficient as soon as GSO is involved.

On a GSO only workload (UDP or TCP), this patch series
can save about 8 % of cpu cycles on a 40Gbit mlx4 NIC,
by keeping optimal batching, and avoiding expensive
qdisc requeues and reschedules.

This patch series :

- Add netdev_tx_sent_queue_more() so that drivers
  can implement efficient BQL and xmit_more support.

- Implement a work around in dev_hard_start_xmit()
  for drivers not using netdev_tx_sent_queue_more()

- changes mlx4 to use netdev_tx_sent_queue_more()

Eric Dumazet (3):
  net: bql: add netdev_tx_sent_queue_more() helper
  net: do not abort bulk send on BQL status
  net/mlx4_en: use netdev_tx_sent_queue_more()

 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 10 ++++++++--
 include/linux/netdevice.h                  | 12 ++++++++++++
 net/core/dev.c                             |  2 +-
 3 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.19.1.568.g152ad8e336-goog

[PATCH net 1/3] net: bql: add netdev_tx_sent_queue_more() helper

[Eric Dumazet]

When qdisc_run() tries to use BQL budget to bulk-dequeue a batch
of packets, GSO can later transform this list in another list
of skbs, and each skb is sent to device ndo_start_xmit(),
one at a time, with skb->xmit_more being set to one but
for last skb.

Problem is that very often, BQL limit is hit in the middle of
the packet train, forcing dev_hard_start_xmit() to stop the
bulk send and requeue the end of the list.

BQL role is to avoid head of line blocking, making sure
a qdisc can deliver high priority packets before low priority ones.

But there is no way requeued packets can be bypassed by fresh
packets in the qdisc.

Aborting the bulk send increases TX softirqs, and hot cache
lines (after skb_segment()) are wasted.

Note that for TSO packets, we never split a packet in the middle
because of BQL limit being hit.

Drivers should be able to update BQL counters without
flipping/caring about BQL status, if the current skb
has xmit_more set.

Upper layers are ultimately responsible to stop sending another
packet train when BQL limit is hit.

Code template in a driver might look like the following :

	if (skb->xmit_more) {
		netdev_tx_sent_queue_more(tx_queue, nr_bytes);
		send_doorbell = netif_tx_queue_stopped(tx_queue);
	} else {
		netdev_tx_sent_queue(tx_queue, nr_bytes);
		send_doorbell = true;
	}

Note that netdev_tx_sent_queue_more() use is not mandatory,
since following patch will change dev_hard_start_xmit()
to not care about BQL status.

But it is higly recommended so that xmit_more full benefits
can be reached (less doorbells sent, and less atomic operations as well)

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/netdevice.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index dc1d9ed33b3192e9406b17c3107b3235b28ff1b9..beb37232688f7e4a71c932e472454e94df18b865 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3166,6 +3166,18 @@ static inline void netdev_txq_bql_complete_prefetchw(struct netdev_queue *dev_qu
 #endif
 }
 
+/* Variant of netdev_tx_sent_queue() for packets with xmit_more.
+ * We do want to change __QUEUE_STATE_STACK_XOFF only for the last
+ * skb of a batch.
+ */
+static inline void netdev_tx_sent_queue_more(struct netdev_queue *dev_queue,
+					     unsigned int bytes)
+{
+#ifdef CONFIG_BQL
+	dql_queued(&dev_queue->dql, bytes);
+#endif
+}
+
 static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
 					unsigned int bytes)
 {
-- 
2.19.1.568.g152ad8e336-goog

[PATCH net 2/3] net: do not abort bulk send on BQL status

[Eric Dumazet]

Before calling dev_hard_start_xmit(), upper layers tried
to cook optimal skb list based on BQL budget.

Problem is that GSO packets can end up comsuming more than
the BQL budget.

Breaking the loop is not useful, since requeued packets
are ahead of any packets still in the qdisc.

It is also more expensive, since next TX completion will
push these packets later, while skbs are not in cpu caches.

It is also a behavior difference with TSO packets, that can
break the BQL limit by a large amount.

Note that drivers should use netdev_tx_sent_queue_more()
in order to have optimal xmit_more support, and avoid
useless atomic operations as shown in the following patch.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 77d43ae2a7bbe1267f8430d5c35637d1984f463c..0ffcbdd55fa9ee545c807f2ed3fc178830e3075a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3272,7 +3272,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *first, struct net_device *de
 		}
 
 		skb = next;
-		if (netif_xmit_stopped(txq) && skb) {
+		if (netif_tx_queue_stopped(txq) && skb) {
 			rc = NETDEV_TX_BUSY;
 			break;
 		}
-- 
2.19.1.568.g152ad8e336-goog

[PATCH net 3/3] net/mlx4_en: use netdev_tx_sent_queue_more()

[Eric Dumazet]

This patch has two changes :

1) Use netdev_tx_sent_queue_more() for skbs with xmit_more
   This avoids mangling BQL status, since we only need to
   take care of it for the last skb of the batch.

2) doorbel only depends on xmit_more and netif_tx_queue_stopped()

  While not strictly necessary after 1), it is more consistent
  this way.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 1857ee0f0871d48285a6d3711f7c3e9a1e08a05f..3acce02ade6a115881ecd72e4710e332d3f380cb 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -1006,7 +1006,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		ring->packets++;
 	}
 	ring->bytes += tx_info->nr_bytes;
-	netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
 	AVG_PERF_COUNTER(priv->pstats.tx_pktsz_avg, skb->len);
 
 	if (tx_info->inl)
@@ -1044,7 +1043,14 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		netif_tx_stop_queue(ring->tx_queue);
 		ring->queue_stopped++;
 	}
-	send_doorbell = !skb->xmit_more || netif_xmit_stopped(ring->tx_queue);
+
+	if (skb->xmit_more) {
+		netdev_tx_sent_queue_more(ring->tx_queue, tx_info->nr_bytes);
+		send_doorbell = netif_tx_queue_stopped(ring->tx_queue);
+	} else {
+		netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
+		send_doorbell = true;
+	}
 
 	real_size = (real_size / 16) & 0x3f;
 
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH net 1/3] net: bql: add netdev_tx_sent_queue_more() helper

[Tariq Toukan]

On 30/10/2018 1:25 AM, Eric Dumazet wrote:
> When qdisc_run() tries to use BQL budget to bulk-dequeue a batch
> of packets, GSO can later transform this list in another list
> of skbs, and each skb is sent to device ndo_start_xmit(),
> one at a time, with skb->xmit_more being set to one but
> for last skb.
> 
> Problem is that very often, BQL limit is hit in the middle of
> the packet train, forcing dev_hard_start_xmit() to stop the
> bulk send and requeue the end of the list.
> 
> BQL role is to avoid head of line blocking, making sure
> a qdisc can deliver high priority packets before low priority ones.
> 
> But there is no way requeued packets can be bypassed by fresh
> packets in the qdisc.
> 
> Aborting the bulk send increases TX softirqs, and hot cache
> lines (after skb_segment()) are wasted.
> 
> Note that for TSO packets, we never split a packet in the middle
> because of BQL limit being hit.
> 
> Drivers should be able to update BQL counters without
> flipping/caring about BQL status, if the current skb
> has xmit_more set.
> 
> Upper layers are ultimately responsible to stop sending another
> packet train when BQL limit is hit.
> 
> Code template in a driver might look like the following :
> 
> 	if (skb->xmit_more) {
> 		netdev_tx_sent_queue_more(tx_queue, nr_bytes);
> 		send_doorbell = netif_tx_queue_stopped(tx_queue);
> 	} else {
> 		netdev_tx_sent_queue(tx_queue, nr_bytes);
> 		send_doorbell = true;
> 	}
> 

Hi Eric,
Nice optimization.

I thought of another way of implementing it, by just extending the 
existing netdev_tx_sent_queue function with a new xmit_more parameter, 
that the driver passes.
Something like this:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d837dad24b4c..feb9cbcb5759 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3129,12 +3129,12 @@ static inline void 
netdev_txq_bql_complete_prefetchw(struct netdev_queue *dev_qu
  }

  static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
-                                       unsigned int bytes)
+                                       unsigned int bytes, bool more)
  {
  #ifdef CONFIG_BQL
         dql_queued(&dev_queue->dql, bytes);

-       if (likely(dql_avail(&dev_queue->dql) >= 0))
+       if (more || likely(dql_avail(&dev_queue->dql) >= 0))
                 return;

         set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);


This unifies and simplifies both the stack and driver code, as the new 
suggested function netdev_tx_sent_queue_more can become a private case 
of the existing one.
This would, however, require a one-time maintenance of 31 existing 
usages of the function:
$ git grep netdev_tx_sent_queue drivers/net/ethernet/ | wc -l
31

What do you think?

> Note that netdev_tx_sent_queue_more() use is not mandatory,
> since following patch will change dev_hard_start_xmit()
> to not care about BQL status.

OK so changing the driver code according to the suggested here becomes 
safe starting next patch, and you do it only in patch 3, so it's fine.

> 
> But it is higly recommended so that xmit_more full benefits

typo: highly, just in case you re-spin.

> can be reached (less doorbells sent, and less atomic operations as well)
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>   include/linux/netdevice.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index dc1d9ed33b3192e9406b17c3107b3235b28ff1b9..beb37232688f7e4a71c932e472454e94df18b865 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3166,6 +3166,18 @@ static inline void netdev_txq_bql_complete_prefetchw(struct netdev_queue *dev_qu
>   #endif
>   }
>   
> +/* Variant of netdev_tx_sent_queue() for packets with xmit_more.
> + * We do want to change __QUEUE_STATE_STACK_XOFF only for the last
> + * skb of a batch.
> + */
> +static inline void netdev_tx_sent_queue_more(struct netdev_queue *dev_queue,
> +					     unsigned int bytes)
> +{
> +#ifdef CONFIG_BQL
> +	dql_queued(&dev_queue->dql, bytes);
> +#endif
> +}
> +
>   static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
>   					unsigned int bytes)
>   {
> 

Re: [PATCH net 3/3] net/mlx4_en: use netdev_tx_sent_queue_more()

[Tariq Toukan]

On 30/10/2018 1:25 AM, Eric Dumazet wrote:
> This patch has two changes :
> 
> 1) Use netdev_tx_sent_queue_more() for skbs with xmit_more
>     This avoids mangling BQL status, since we only need to
>     take care of it for the last skb of the batch.
> 
> 2) doorbel only depends on xmit_more and netif_tx_queue_stopped()
> 
>    While not strictly necessary after 1), it is more consistent
>    this way.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_tx.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index 1857ee0f0871d48285a6d3711f7c3e9a1e08a05f..3acce02ade6a115881ecd72e4710e332d3f380cb 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -1006,7 +1006,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>   		ring->packets++;
>   	}
>   	ring->bytes += tx_info->nr_bytes;
> -	netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
>   	AVG_PERF_COUNTER(priv->pstats.tx_pktsz_avg, skb->len);
>   
>   	if (tx_info->inl)
> @@ -1044,7 +1043,14 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>   		netif_tx_stop_queue(ring->tx_queue);
>   		ring->queue_stopped++;
>   	}
> -	send_doorbell = !skb->xmit_more || netif_xmit_stopped(ring->tx_queue);
> +
> +	if (skb->xmit_more) {
> +		netdev_tx_sent_queue_more(ring->tx_queue, tx_info->nr_bytes);
> +		send_doorbell = netif_tx_queue_stopped(ring->tx_queue);
> +	} else {
> +		netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
> +		send_doorbell = true;
> +	}
>   
>   	real_size = (real_size / 16) & 0x3f;
>   
> 

The drivers' code template would be nicer if we unify the two functions 
netdev_tx_sent_queue/netdev_tx_sent_queue_more to a single one with a 
parameter.

Currently, all drivers that would want to benefit from this optimization 
will have to repeat these if/else blocks.

Regards,
Tariq

Re: [PATCH net 1/3] net: bql: add netdev_tx_sent_queue_more() helper

[Eric Dumazet]

On Wed, Oct 31, 2018 at 4:30 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>
>
>
> On 30/10/2018 1:25 AM, Eric Dumazet wrote:
> > When qdisc_run() tries to use BQL budget to bulk-dequeue a batch
> > of packets, GSO can later transform this list in another list
> > of skbs, and each skb is sent to device ndo_start_xmit(),
> > one at a time, with skb->xmit_more being set to one but
> > for last skb.
> >
> > Problem is that very often, BQL limit is hit in the middle of
> > the packet train, forcing dev_hard_start_xmit() to stop the
> > bulk send and requeue the end of the list.
> >
> > BQL role is to avoid head of line blocking, making sure
> > a qdisc can deliver high priority packets before low priority ones.
> >
> > But there is no way requeued packets can be bypassed by fresh
> > packets in the qdisc.
> >
> > Aborting the bulk send increases TX softirqs, and hot cache
> > lines (after skb_segment()) are wasted.
> >
> > Note that for TSO packets, we never split a packet in the middle
> > because of BQL limit being hit.
> >
> > Drivers should be able to update BQL counters without
> > flipping/caring about BQL status, if the current skb
> > has xmit_more set.
> >
> > Upper layers are ultimately responsible to stop sending another
> > packet train when BQL limit is hit.
> >
> > Code template in a driver might look like the following :
> >
> >       if (skb->xmit_more) {
> >               netdev_tx_sent_queue_more(tx_queue, nr_bytes);
> >               send_doorbell = netif_tx_queue_stopped(tx_queue);
> >       } else {
> >               netdev_tx_sent_queue(tx_queue, nr_bytes);
> >               send_doorbell = true;
> >       }
> >
>
> Hi Eric,
> Nice optimization.
>
> I thought of another way of implementing it, by just extending the
> existing netdev_tx_sent_queue function with a new xmit_more parameter,
> that the driver passes.
> Something like this:
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d837dad24b4c..feb9cbcb5759 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3129,12 +3129,12 @@ static inline void
> netdev_txq_bql_complete_prefetchw(struct netdev_queue *dev_qu
>   }
>
>   static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
> -                                       unsigned int bytes)
> +                                       unsigned int bytes, bool more)
>   {
>   #ifdef CONFIG_BQL
>          dql_queued(&dev_queue->dql, bytes);
>
> -       if (likely(dql_avail(&dev_queue->dql) >= 0))
> +       if (more || likely(dql_avail(&dev_queue->dql) >= 0))
>                  return;
>
>          set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);
>
>
> This unifies and simplifies both the stack and driver code, as the new
> suggested function netdev_tx_sent_queue_more can become a private case
> of the existing one.

Yes I thought of this, but it is too risky/invasive and I prefer
adding an opt-in mechanism
so that patch authors can test their patch.

Then when/if all drivers are updated, we can remove the legacy stuff
or rename everything.

mlx4 was the only NIC I could reasonably test myself.

Another suggestion from Willem is to add the following helper, returning
a boolean (doorbell needed)

+static inline bool __netdev_tx_sent_queue(struct netdev_queue *dev_queue,
+                                         unsigned int bytes, bool xmit_more)
+{
+       if (xmit_more) {
+               netdev_tx_sent_queue_more(ring->tx_queue, tx_info->nr_bytes);
+               return netif_tx_queue_stopped(dev_queue);
+       } else {
+               netdev_tx_sent_queue(dev_queue, bytes);
+               return true;
+       }
+}
+

Re: [PATCH net 3/3] net/mlx4_en: use netdev_tx_sent_queue_more()

[Eric Dumazet]

On Wed, Oct 31, 2018 at 4:37 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>
>
>
> On 30/10/2018 1:25 AM, Eric Dumazet wrote:
> > This patch has two changes :
> >
> > 1) Use netdev_tx_sent_queue_more() for skbs with xmit_more
> >     This avoids mangling BQL status, since we only need to
> >     take care of it for the last skb of the batch.
> >
> > 2) doorbel only depends on xmit_more and netif_tx_queue_stopped()
> >
> >    While not strictly necessary after 1), it is more consistent
> >    this way.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Tariq Toukan <tariqt@mellanox.com>
> > ---
> >   drivers/net/ethernet/mellanox/mlx4/en_tx.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > index 1857ee0f0871d48285a6d3711f7c3e9a1e08a05f..3acce02ade6a115881ecd72e4710e332d3f380cb 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > @@ -1006,7 +1006,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
> >               ring->packets++;
> >       }
> >       ring->bytes += tx_info->nr_bytes;
> > -     netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
> >       AVG_PERF_COUNTER(priv->pstats.tx_pktsz_avg, skb->len);
> >
> >       if (tx_info->inl)
> > @@ -1044,7 +1043,14 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
> >               netif_tx_stop_queue(ring->tx_queue);
> >               ring->queue_stopped++;
> >       }
> > -     send_doorbell = !skb->xmit_more || netif_xmit_stopped(ring->tx_queue);
> > +
> > +     if (skb->xmit_more) {
> > +             netdev_tx_sent_queue_more(ring->tx_queue, tx_info->nr_bytes);
> > +             send_doorbell = netif_tx_queue_stopped(ring->tx_queue);
> > +     } else {
> > +             netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
> > +             send_doorbell = true;
> > +     }
> >
> >       real_size = (real_size / 16) & 0x3f;
> >
> >
>
> The drivers' code template would be nicer if we unify the two functions
> netdev_tx_sent_queue/netdev_tx_sent_queue_more to a single one with a
> parameter.
>
> Currently, all drivers that would want to benefit from this optimization
> will have to repeat these if/else blocks.

I can add a helper sure, but I can not change drivers that I am not
able to test.

So I can not change existing helper.

This patch series shows the problem and fixes one driver, a common
helper can be added when a second
driver is updated, there is no hurry.