Message-ID: <20251021155628.7383bfca@kernel.org>
Date: Tue, 21 Oct 2025 15:56:28 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: David Wilder <wilder@...ibm.com>
Cc: Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org"
 <netdev@...r.kernel.org>, "jv@...sburgh.net" <jv@...sburgh.net>, Pradeep
 Satyanarayana <pradeep@...ibm.com>, "i.maximets@....org"
 <i.maximets@....org>, Adrian Moreno Zapata <amorenoz@...hat.com>, Hangbin
 Liu <haliu@...hat.com>, "stephen@...workplumber.org"
 <stephen@...workplumber.org>, "horms@...nel.org" <horms@...nel.org>,
 "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "edumazet@...gle.com"
 <edumazet@...gle.com>, Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCH net-next v13 6/7] bonding: Update for extended
 arp_ip_target format.

On Fri, 17 Oct 2025 00:21:02 +0000 David Wilder wrote:
> > > I guess you should update bond_get_size() accordingly???
> > >
> > > Also changing the binary layout of an existing NL type does not feel
> > > safe. @Jakub: is that something we can safely allow?  
> >
> > In general extending attributes is fine, but going from a scalar
> > to a struct is questionable. YNL for example will not allow it.  
> 
> I am not sure I understand your concern. I have change the
> netlink socket payload from a fixed 4 bytes to a variable number of bytes.
> 4 bytes for ipv4 address followed by some number of bytes with the
> list of vlans, could be zero. Netlink sockets just need to be told the
> payload size.  Or have I missed the point?

Are you replacing a line that says nla_put() which outputs raw bytes
or a line which says nla_put_x32() which outputs a scalar?
What I'm saying is that while growing raw byte attrs is pretty common
in Netlink, replacing a scalar with a struct may cause user space
to reject the attrs.

> > I haven't looked at the series more closely until now.
> >
> > Why are there multiple vlan tags per target?  
> 
> You can have a vlan inside a vlan, the original arp_ip_target
> option code supported this.
> 
> > Is this configuration really something we should support in the kernel?
> > IDK how much we should push "OvS-compatibility" into other parts of the
> > stack. If user knows that they have to apply this funny configuration
> > on the bond maybe they should just arp from user space?  
> 
> This change is not just for compatibility with OVS. Ilya Maximets pointed out:
> "..this is true not only for OVS.  You can add various TC qdiscs onto the
> interface that will break all those assumptions as well, for example.  Loaded
> BPF/XDP programs will too."
> 
> When using the arp_ip_target option the bond driver must discover what
> vlans are in the path to the target. These special arps must be generated by
> the bonding driver to know what bonded slave the packets is to sent and
> received on and at what frequency.
> 
> When the the arp_ip_target feature was first introduced discovering vlans in the
> path to the target was easy by following the linked net_devices. As our
> networking code has evolved this is no longer possible with all configurations
> as Ilya pointed out.  What I have done is provide alternate way to provide the
> list of vlans so this desirable feature can continue to function.

I understand your perspective. I'm not convinced that kernel must
support such custom configurations, if it can't infer the correct
behavior from information it already has.

I don't feel strongly about it, if you manage to collect a review 
tag from the bonding maintainers or another netdev maintainer I won't
stand in the way. Otherwise, given that the uAPI is questionable and
there's total of 0 review tags on v13, this series is starting to look
like a dead end.

Powered by blists - more mailing lists