veth: Add updating of trans_start

PATCH Applied Jay Vosburgh 3 participants 10 messages Jun 16, 2022 – Jun 22, 2022

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Jay Vosburgh

author

2022-06-16 19:26 UTC #0 lore raw

PATCH 1/1 veth: Add updating of trans_start

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 <[email protected]>

Tested-by Jonathan Toppins <[email protected]>

Signed-off-by Jay Vosburgh <[email protected]>

Fixes 21a75f0915dd ("bonding: Fix ARP monitor validation")

Link https://lore.kernel.org/netdev/[email protected]/ open here

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

drivers/net/veth.c +4 -0

index 466da01ba2e3..2cb833b3006a 100644
@@ -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 {

Signature · 1 line

2.29.GIT

[email protected]

CI Applied

2022-06-17 10:40 UTC #1 lore raw

This patch was applied to netdev/net.git (master) by David S. Miller <[email protected]>:

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:

Quoted 9 lines · 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!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Jakub Kicinski

maintainer

2022-06-17 15:45 UTC #2 lore raw

In reply to Jay Vosburgh

Quoted 5 lines · 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?

Jay Vosburgh

author

2022-06-17 16:43 UTC #3 lore raw

Quoted 10 lines · Jakub Kicinski

Quoted 5 lines · 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, [email protected]

Jakub Kicinski

maintainer

2022-06-17 19:44 UTC #4 lore raw

Quoted 6 lines · 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?

Quoted 8 lines

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.

Jay Vosburgh

author

2022-06-18 00:27 UTC #5 lore raw

Quoted 11 lines · Jakub Kicinski

Quoted 6 lines · 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.

Quoted 9 lines

Quoted 8 lines
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);

Quoted 5 lines

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, [email protected]

Jakub Kicinski

maintainer

2022-06-18 00:56 UTC #6 lore raw

Quoted 20 lines · On Fri, 17 Jun 2022 17:27:43 -0700 Jay Vosburgh wrote:

Quoted 11 lines · Jakub Kicinski
Quoted 6 lines · 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).

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
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);  

Quoted 8 lines

Quoted 5 lines
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.

Jakub Kicinski

maintainer

2022-06-21 19:52 UTC #7 lore raw

Quoted 16 lines · On Fri, 17 Jun 2022 17:55:50 -0700 Jakub Kicinski wrote:

Quoted 11 lines
Quoted 3 lines
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).
Quoted 4 lines
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.

Quoted 8 lines

Right, unfortunately.
Quoted 2 lines
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?

Jay Vosburgh

author

2022-06-22 01:42 UTC #8 lore raw

Quoted 22 lines · Jakub Kicinski

Quoted 21 lines · On Fri, 17 Jun 2022 17:55:50 -0700 Jakub Kicinski wrote:
Quoted 11 lines
Quoted 3 lines
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).
Quoted 4 lines
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.
Quoted 2 lines
The question then becomes how to distinguish a software LLTX
device from a hardware LLTX device.

Quoted 6 lines

Quoted 3 lines
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, [email protected]

Jakub Kicinski

maintainer

2022-06-22 04:38 UTC #9 lore raw

Quoted 11 lines · 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!