All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: David Wilder <wilder@us.ibm.com>
Cc: Paolo Abeni <pabeni@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"jv@jvosburgh.net" <jv@jvosburgh.net>,
	Pradeep Satyanarayana <pradeep@us.ibm.com>,
	"i.maximets@ovn.org" <i.maximets@ovn.org>,
	Adrian Moreno Zapata <amorenoz@redhat.com>,
	Hangbin Liu <haliu@redhat.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	"horms@kernel.org" <horms@kernel.org>,
	"andrew+netdev@lunn.ch" <andrew+netdev@lunn.ch>,
	"edumazet@google.com" <edumazet@google.com>,
	Nikolay Aleksandrov <razor@blackwall.org>
Subject: Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.
Date: Tue, 21 Oct 2025 15:56:28 -0700	[thread overview]
Message-ID: <20251021155628.7383bfca@kernel.org> (raw)
In-Reply-To: <MW3PR15MB3913E83123930C417DDD1AC8FAE9A@MW3PR15MB3913.namprd15.prod.outlook.com>

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.

  parent reply	other threads:[~2025-10-21 22:56 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-13 23:52 [PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to allow for a list of vlan tags David Wilder
2025-10-13 23:52 ` [PATCH net-next v13 1/7] bonding: Adding struct bond_arp_target David Wilder
2025-10-13 23:52 ` [PATCH net-next v13 2/7] bonding: Adding extra_len field to struct bond_opt_value David Wilder
2025-10-13 23:52 ` [PATCH net-next v13 3/7] bonding: arp_ip_target helpers David Wilder
2025-10-16 11:14   ` Paolo Abeni
2025-10-13 23:52 ` [PATCH net-next v13 4/7] bonding: Processing extended arp_ip_target from user space David Wilder
2025-10-16 11:26   ` Paolo Abeni
2025-10-13 23:52 ` [PATCH net-next v13 5/7] bonding: Update to bond_arp_send_all() to use supplied vlan tags David Wilder
2025-10-16 11:38   ` Paolo Abeni
2025-10-13 23:52 ` [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format David Wilder
2025-10-16 11:50   ` Paolo Abeni
2025-10-16 19:49     ` Jakub Kicinski
2025-10-17  0:21       ` David Wilder
2025-10-21 16:00         ` David Wilder
2025-10-21 16:18           ` David Wilder
2025-10-21 22:56         ` Jakub Kicinski [this message]
2025-10-13 23:52 ` [PATCH net-next v13 7/7] bonding: Selftest and documentation for the arp_ip_target parameter David Wilder
2025-10-16 11:55   ` Paolo Abeni
2025-10-16 11:48 ` [PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to allow for a list of vlan tags Nikolay Aleksandrov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20251021155628.7383bfca@kernel.org \
    --to=kuba@kernel.org \
    --cc=amorenoz@redhat.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=edumazet@google.com \
    --cc=haliu@redhat.com \
    --cc=horms@kernel.org \
    --cc=i.maximets@ovn.org \
    --cc=jv@jvosburgh.net \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=pradeep@us.ibm.com \
    --cc=razor@blackwall.org \
    --cc=stephen@networkplumber.org \
    --cc=wilder@us.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.