On Wed, Jun 22, 2022 at 11:10:17AM +0800, Tao Liu wrote:
> On Tue, Jun 21, 2022 at 06:49:42AM -0700, Marcelo Ricardo Leitner wrote:
> > 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?
> >
> Hi, thanks for your reply.
>
> We are using BlueField2 in Bare Metal Server.
> p0 and p1 enslave to master bond0. A SF is created for RDMA.
> ovs manages pf0hpf and gre.
>
> Traffics between bond0 and SF are controlled by tc rules.
> ```
> master=bond0
> slave1=p0
> slave2=p1
> sf=enp3s0f0s0
> sf_rep=en3f0pf0sf0
> $tc qdisc add dev $master ingress_block 22 ingress
> $tc qdisc add dev $slave1 ingress_block 22 ingress
> $tc qdisc add dev $slave2 ingress_block 22 ingress
> $tc qdisc add dev $sf_rep ingress
>
> # some customized tc rules
> $tc filter add dev $sf_rep pref 1 ingress ... action mirred egress redirect
> dev $master
>
> $tc filter add block 22 pref 1 ... action mirred egress redirect dev $sf_rep
> ```
>
> Unfortunately ingress_block on p0 or p1 may be deleted and recreated by
> ovs when they enslaves to bond0.
>
> Also we have a similar architectur on cx5/cx6.
>
> > >
> > > If bond master is not attached to ovs, the ingress block on slaves shoud
> > > not be updated.
> >
> > Why? (in short, but more below)
> >
> If we do not use bond master and slaves in ovs, they shoud not be interfered.
Makes sense, ...
>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
> >
> This patch does not break the tunnel use case. The ingress attaches on
> vxlan_sys, but not need to attach on bond master or slaves.
... I was under the impression that the driver needed it somehow,
despite that. But apparently that's not right, as the driver will have
one indr callback for each uplink, and should be able to have its way
from there.
Thanks for the explanations.
Reviewed-by: Marcelo Ricardo Leitner <[email protected]>
>
> > > + 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
