Hi, On Tue, Jun 21, 2022 at 02:10:39PM +0800, Tao Liu wrote: > Bond master netdev may be created without a classification type, due > to routing or tunneling code.
Can you please elaborate on why is this an issue? > > If bond master is not attached to ovs, the ingress block on slaves shoud > not be updated. Why? (in short, but more below) > > Simple reproducer: > ip a add 10.1.1.1/30 dev bond0 > ip l set net3 master bond0 > tc q ls dev net3 > > Fixes: d22f8927c3c9 ("netdev-linux: monitor and offload LAG slaves to TC") > Signed-off-by: Tao Liu <thomas....@ucloud.cn> > --- > lib/netdev-linux.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 9d12502..b9e0c99 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -682,7 +682,10 @@ netdev_linux_update_lag(struct rtnetlink_change *change) > return; > } > > - if (is_netdev_linux_class(master_netdev->netdev_class)) { > + /* If bond master is not attached to ovs, ingress block on slaves > + * shoud not be updated. */ I think this will break a core use case. As in your reproducer, that's pretty much how it is expected to work today with tunnels, for example: ip a add 10.1.1.1/30 dev bond0 ip l set net3 master bond0 ip l s bond0 up ovs-vsctl add-port ovsbr0 vxlan0 -- \ set interface vxlan0 type=vxlan \ options:local_ip=10.1.1.1 options:remote_ip=10.1.1.2 options:key=0 If you patch like this, then who would be adding the ingress qdiscs on the slaves? Forcing the bond to be added is probably not optimal, because it doesn't really need to be. Unless your considering that as some sort of authorization for ovs to mangle with it? Marcelo > + if (!master_netdev->auto_classified && > + is_netdev_linux_class(master_netdev->netdev_class)) { > block_id = netdev_get_block_id(master_netdev); > if (!block_id) { > netdev_close(master_netdev); > -- > 1.8.3.1 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev