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
