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.
> >
> > 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.
> > + 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
