[PATCH net] veth: Add updating of trans_start

[PATCH net] veth: Add updating of trans_start

[Jay Vosburgh]

	Since commit 21a75f0915dd ("bonding: Fix ARP monitor validation"),
the bonding ARP / ND link monitors depend on the trans_start time to
determine link availability.  NETIF_F_LLTX drivers must update trans_start
directly, which veth does not do.  This prevents use of the ARP or ND link
monitors with veth interfaces in a bond.

	Resolve this by having veth_xmit update the trans_start time.

Reported-by: Jonathan Toppins <jtoppins@redhat.com>
Tested-by: Jonathan Toppins <jtoppins@redhat.com>
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Fixes: 21a75f0915dd ("bonding: Fix ARP monitor validation")
Link: https://lore.kernel.org/netdev/b2fd4147-8f50-bebd-963a-1a3e8d1d9715@redhat.com/

---
 drivers/net/veth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 466da01ba2e3..2cb833b3006a 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -312,6 +312,7 @@ static bool veth_skb_is_eligible_for_gro(const struct net_device *dev,
 static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
+	struct netdev_queue *queue = NULL;
 	struct veth_rq *rq = NULL;
 	struct net_device *rcv;
 	int length = skb->len;
@@ -329,6 +330,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 	rxq = skb_get_queue_mapping(skb);
 	if (rxq < rcv->real_num_rx_queues) {
 		rq = &rcv_priv->rq[rxq];
+		queue = netdev_get_tx_queue(dev, rxq);
 
 		/* The napi pointer is available when an XDP program is
 		 * attached or when GRO is enabled
@@ -340,6 +342,8 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb_tx_timestamp(skb);
 	if (likely(veth_forward_skb(rcv, skb, rq, use_napi) == NET_RX_SUCCESS)) {
+		if (queue)
+			txq_trans_cond_update(queue);
 		if (!use_napi)
 			dev_lstats_add(dev, length);
 	} else {
-- 
2.29.GIT

Re: [PATCH net] veth: Add updating of trans_start

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 16 Jun 2022 12:26:30 -0700 you wrote:
> Since commit 21a75f0915dd ("bonding: Fix ARP monitor validation"),
> the bonding ARP / ND link monitors depend on the trans_start time to
> determine link availability.  NETIF_F_LLTX drivers must update trans_start
> directly, which veth does not do.  This prevents use of the ARP or ND link
> monitors with veth interfaces in a bond.
> 
> 	Resolve this by having veth_xmit update the trans_start time.
> 
> [...]

Here is the summary with links:
  - [net] veth: Add updating of trans_start
    https://git.kernel.org/netdev/net/c/e66e257a5d83

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] veth: Add updating of trans_start

[Jakub Kicinski]

On Thu, 16 Jun 2022 12:26:30 -0700 Jay Vosburgh wrote:
> 	Since commit 21a75f0915dd ("bonding: Fix ARP monitor validation"),
> the bonding ARP / ND link monitors depend on the trans_start time to
> determine link availability.  NETIF_F_LLTX drivers must update trans_start
> directly, which veth does not do.  This prevents use of the ARP or ND link
> monitors with veth interfaces in a bond.

Why is a SW device required to update its trans_start? trans_start is
for the Tx hang watchdog, AFAIK, not a general use attribute. There's
plenty of NETIF_F_LLTX devices, are they all broken? 

Re: [PATCH net] veth: Add updating of trans_start

[Jay Vosburgh]

Jakub Kicinski <kuba@kernel.org> wrote:

>On Thu, 16 Jun 2022 12:26:30 -0700 Jay Vosburgh wrote:
>> 	Since commit 21a75f0915dd ("bonding: Fix ARP monitor validation"),
>> the bonding ARP / ND link monitors depend on the trans_start time to
>> determine link availability.  NETIF_F_LLTX drivers must update trans_start
>> directly, which veth does not do.  This prevents use of the ARP or ND link
>> monitors with veth interfaces in a bond.
>
>Why is a SW device required to update its trans_start? trans_start is
>for the Tx hang watchdog, AFAIK, not a general use attribute. There's
>plenty of NETIF_F_LLTX devices, are they all broken? 

	In this case, it's to permit the bonding ARP / ND monitor to
function if that software device (veth in this case) is added to a bond
using the ARP / ND monitor (which relies on trans_start, and has done so
since at least 2.6.0).  I'll agree it's a niche case; this was broken
for veth for quite some time, but veth + netns is handy for software
only test cases, so it seems worth doing.

	I didn't exhaustively check all LLTX drivers, but, e.g., tun
does update trans_start:

drivers/net/tun.c:

       /* NETIF_F_LLTX requires to do our own update of trans_start */
        queue = netdev_get_tx_queue(dev, txq);
        txq_trans_cond_update(queue);

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] veth: Add updating of trans_start

[Jakub Kicinski]

On Fri, 17 Jun 2022 09:42:55 -0700 Jay Vosburgh wrote:
> 	In this case, it's to permit the bonding ARP / ND monitor to
> function if that software device (veth in this case) is added to a bond
> using the ARP / ND monitor (which relies on trans_start, and has done so
> since at least 2.6.0).  I'll agree it's a niche case; this was broken
> for veth for quite some time, but veth + netns is handy for software
> only test cases, so it seems worth doing.

I presume it needs it to check if the device has transmitted anything
in the last unit of time, can we look at the device stats for LLTX for
example?

> 	I didn't exhaustively check all LLTX drivers, but, e.g., tun
> does update trans_start:
> 
> drivers/net/tun.c:
> 
>        /* NETIF_F_LLTX requires to do our own update of trans_start */
>         queue = netdev_get_tx_queue(dev, txq);
>         txq_trans_cond_update(queue);

Well, it is _an_ example, but the only one I can find. And the
justification is the same as yours now -- make bonding work a31d27fb.
Because of that I don't think we can use tun as a proof that trans 
start should be updated on LLTX devices as a general, stack-wide rule.
There's a lot more LLTX devices than veth and tun.

Re: [PATCH net] veth: Add updating of trans_start

[Jay Vosburgh]

Jakub Kicinski <kuba@kernel.org> wrote:

>On Fri, 17 Jun 2022 09:42:55 -0700 Jay Vosburgh wrote:
>> 	In this case, it's to permit the bonding ARP / ND monitor to
>> function if that software device (veth in this case) is added to a bond
>> using the ARP / ND monitor (which relies on trans_start, and has done so
>> since at least 2.6.0).  I'll agree it's a niche case; this was broken
>> for veth for quite some time, but veth + netns is handy for software
>> only test cases, so it seems worth doing.
>
>I presume it needs it to check if the device has transmitted anything
>in the last unit of time, can we look at the device stats for LLTX for
>example?

	Yes, that's the use case.  

	Hmm.  Polling the device stats would likely work for software
devices, although the unit of time varies (some checks are fixed at one
unit, but others can be N units depending on the missed_max option
setting).

	Polling hardware devices might not work; as I recall, some
devices only update the statistics on timespans on the order of seconds,
e.g., bnx2 and tg3 appear to update once per second.  But those do
update trans_start.

	The question then becomes how to distinguish a software LLTX
device from a hardware LLTX device.

>> 	I didn't exhaustively check all LLTX drivers, but, e.g., tun
>> does update trans_start:
>> 
>> drivers/net/tun.c:
>> 
>>        /* NETIF_F_LLTX requires to do our own update of trans_start */
>>         queue = netdev_get_tx_queue(dev, txq);
>>         txq_trans_cond_update(queue);
>
>Well, it is _an_ example, but the only one I can find. And the
>justification is the same as yours now -- make bonding work a31d27fb.
>Because of that I don't think we can use tun as a proof that trans 
>start should be updated on LLTX devices as a general, stack-wide rule.
>There's a lot more LLTX devices than veth and tun.

	I'm not suggesting that all (software) LLTX software devices be
updated.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] veth: Add updating of trans_start

[Jakub Kicinski]

On Fri, 17 Jun 2022 17:27:43 -0700 Jay Vosburgh wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> >On Fri, 17 Jun 2022 09:42:55 -0700 Jay Vosburgh wrote:  
> >> 	In this case, it's to permit the bonding ARP / ND monitor to
> >> function if that software device (veth in this case) is added to a bond
> >> using the ARP / ND monitor (which relies on trans_start, and has done so
> >> since at least 2.6.0).  I'll agree it's a niche case; this was broken
> >> for veth for quite some time, but veth + netns is handy for software
> >> only test cases, so it seems worth doing.  
> >
> >I presume it needs it to check if the device has transmitted anything
> >in the last unit of time, can we look at the device stats for LLTX for
> >example?  
> 
> 	Yes, that's the use case.  
> 
> 	Hmm.  Polling the device stats would likely work for software
> devices, although the unit of time varies (some checks are fixed at one
> unit, but others can be N units depending on the missed_max option
> setting).
> 
> 	Polling hardware devices might not work; as I recall, some
> devices only update the statistics on timespans on the order of seconds,
> e.g., bnx2 and tg3 appear to update once per second.  But those do
> update trans_start.

Right, unfortunately.

> 	The question then becomes how to distinguish a software LLTX
> device from a hardware LLTX device.

If my way of thinking about trans_start is correct then we can test 
for presence of ndo_tx_timeout. Anything that has the tx_timeout NDO
must be maintaining trans_start.

> >> 	I didn't exhaustively check all LLTX drivers, but, e.g., tun
> >> does update trans_start:
> >> 
> >> drivers/net/tun.c:
> >> 
> >>        /* NETIF_F_LLTX requires to do our own update of trans_start */
> >>         queue = netdev_get_tx_queue(dev, txq);
> >>         txq_trans_cond_update(queue);  
> >
> >Well, it is _an_ example, but the only one I can find. And the
> >justification is the same as yours now -- make bonding work a31d27fb.
> >Because of that I don't think we can use tun as a proof that trans 
> >start should be updated on LLTX devices as a general, stack-wide rule.
> >There's a lot more LLTX devices than veth and tun.  
> 
> 	I'm not suggesting that all (software) LLTX software devices be
> updated.

The ones which are not updated would remain broken then, no?
Waiting for someone to try to bond them and discover it doesn't work.

Re: [PATCH net] veth: Add updating of trans_start

[Jakub Kicinski]

On Fri, 17 Jun 2022 17:55:50 -0700 Jakub Kicinski wrote:
> > >I presume it needs it to check if the device has transmitted anything
> > >in the last unit of time, can we look at the device stats for LLTX for
> > >example?    
> > 
> > 	Yes, that's the use case.  
> > 
> > 	Hmm.  Polling the device stats would likely work for software
> > devices, although the unit of time varies (some checks are fixed at one
> > unit, but others can be N units depending on the missed_max option
> > setting).
> > 
> > 	Polling hardware devices might not work; as I recall, some
> > devices only update the statistics on timespans on the order of seconds,
> > e.g., bnx2 and tg3 appear to update once per second.  But those do
> > update trans_start.  
> 
> Right, unfortunately.
> 
> > 	The question then becomes how to distinguish a software LLTX
> > device from a hardware LLTX device.  
> 
> If my way of thinking about trans_start is correct then we can test 
> for presence of ndo_tx_timeout. Anything that has the tx_timeout NDO
> must be maintaining trans_start.

So what's your thinking Jay? Keep this as an immediate small fix 
for net but work on using a different approach in net-next?

Re: [PATCH net] veth: Add updating of trans_start

[Jay Vosburgh]

Jakub Kicinski <kuba@kernel.org> wrote:

>On Fri, 17 Jun 2022 17:55:50 -0700 Jakub Kicinski wrote:
>> > >I presume it needs it to check if the device has transmitted anything
>> > >in the last unit of time, can we look at the device stats for LLTX for
>> > >example?    
>> > 
>> > 	Yes, that's the use case.  
>> > 
>> > 	Hmm.  Polling the device stats would likely work for software
>> > devices, although the unit of time varies (some checks are fixed at one
>> > unit, but others can be N units depending on the missed_max option
>> > setting).
>> > 
>> > 	Polling hardware devices might not work; as I recall, some
>> > devices only update the statistics on timespans on the order of seconds,
>> > e.g., bnx2 and tg3 appear to update once per second.  But those do
>> > update trans_start.  
>> 
>> Right, unfortunately.
>> 
>> > 	The question then becomes how to distinguish a software LLTX
>> > device from a hardware LLTX device.  
>> 
>> If my way of thinking about trans_start is correct then we can test 
>> for presence of ndo_tx_timeout. Anything that has the tx_timeout NDO
>> must be maintaining trans_start.
>
>So what's your thinking Jay? Keep this as an immediate small fix 
>for net but work on using a different approach in net-next?

	Sorry, was out for the three day weekend.

	I had a quick look and I think you're probably right that
anything with a ndo_tx_timeout will deal with trans_start, and anything
without ndo_tx_timeout will be a software device not subject to delayed
batching of stats updates.

	And, yes, if there are no objections, what I'd like to do now is
apply the veth change to get things working and work up the bifurcated
approach separately (which would ultimately include removing the
trans_start updates from veth and tun).

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] veth: Add updating of trans_start

[Jakub Kicinski]

On Tue, 21 Jun 2022 18:42:19 -0700 Jay Vosburgh wrote:
> 	Sorry, was out for the three day weekend.
> 
> 	I had a quick look and I think you're probably right that
> anything with a ndo_tx_timeout will deal with trans_start, and anything
> without ndo_tx_timeout will be a software device not subject to delayed
> batching of stats updates.
> 
> 	And, yes, if there are no objections, what I'd like to do now is
> apply the veth change to get things working and work up the bifurcated
> approach separately (which would ultimately include removing the
> trans_start updates from veth and tun).

Works for me, thanks!