On Mon, Jun 27, 2022 at 09:40:38AM +0300, Roi Dayan wrote:
>
>
> On 2022-06-23 12:42 PM, Roi Dayan wrote:
> >
> >
> > On 2022-06-22 11:56 PM, Marcelo Ricardo Leitner wrote:
> > > 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]>
> > >
> >
> > there is an example without bond about ingress. We can
> > use vxlan port in ovs with tunnel ip assigned on the pf.
> > like Tao mentioned, the ingress and tc rules are created on
> > the vxlan netdev created by ovs but ovs doesnt recreate ingress
> > on the pf and the rules are offloaded by the driver.
> >
> > also looks good to me. thanks.
> >
> > Reviewed-by: Roi Dayan <[email protected]>
> >
>
> Hmm, sorry but looks like I did review too quick.
> I did some manual testing now and ovs doesn't add
> ingress to the slave devices for me.
> master_netdev->auto_classified==true in my case.
> can you describe the steps you did to test? or you just tested
> the flow where bond is not attached to ovs?
>
# ovs-vsctl show
866b5d8e-bf4d-4600-9dd3-b724036c7e99
ovs_version: "2.17.90"
# echo "+bond0" > /sys/class/net/bonding_masters
# ip l show dev bond0
21: bond0: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN mode
DEFAULT group default qlen 1000
link/ether 94:29:2f:70:22:9c brd ff:ff:ff:ff:ff:ff
# ip a add 10.1.1.1/30 dev bond0
# ip l set net3 down
# tc q add dev net3 ingress_block 111 ingress
# tc q ls dev net3 ingress
qdisc ingress ffff: parent ffff:fff1 ingress_block 111 ----------------
# ip l set net3 master bond0
# tc q ls dev net3 ingress
qdisc ingress ffff: parent ffff:fff1 ingress_block 22 ----------------
Also add some print in netdev_linux_update_lag():
2022-06-27T07:43:45.324Z|00058|netdev_linux|INFO|LAG: master_name=bond0,
master_ifindex=22, auto_classified=1
>
> >
> > > >
> > > > > > + 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
