Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
On Mon, Jun 27, 2022 at 03:47:34PM +0300, Roi Dayan wrote: > > could it be your bond0 configuration? > beside doing +bond0 into /sys/class/net/bonding_masters how do you > config bond0 and its slave? > > I don't have any static config file and use ip commands > as follows: > > ip link add name bond0 type bond mode $mode miimon 100 > ip link set dev $nic1 down > ip link set dev $nic2 down > ip link set dev $nic1 master bond0 > ip link set dev $nic2 master bond0 > ip link set dev bond0 up > ip link set dev $nic1 up > ip link set dev $nic2 up > > > then adding bond0 to ovs bridge, I see bond0 got ingress block 310 > but the slaves didn't get ingress. > > # tc qdisc show dev bond0 ingress > # ovs-vsctl show > eb7bb34b-bdf6-4b9b-88d8-22ffeaf47630 > ovs_version: "2.17.90" > # ovs-vsctl add-br ov1 > # ovs-vsctl add-port ov1 bond0 > # tc qdisc show dev bond0 ingress > qdisc ingress : parent :fff1 ingress_block 310 > # tc qdisc show dev enp8s0f0 ingress > # tc qdisc show dev enp8s0f1 ingress > # ovs-vsctl show > eb7bb34b-bdf6-4b9b-88d8-22ffeaf47630 > Bridge ov1 > Port ov1 > Interface ov1 > type: internal > Port bond0 > Interface bond0 > ovs_version: "2.17.90" > # tc qdisc show dev bond0 ingress > qdisc ingress : parent :fff1 ingress_block 310 > # tc qdisc show dev enp8s0f0 ingress > # tc qdisc show dev enp8s0f1 ingress > # > Hi, Roi. Thanks for having tested this patch. I have reproduced your test case. It is because bond0 first opened with type == NULL, then auto_classified == true. When bond0 adds to ovs, bond0 opens with type == "system", and auto_classified will not be cleared. Will send v3. > > printing in the log showing master_netdev->auto_classified is 1. > you showed in your prev test its 1 as well . so how it got to be > 0 now for you to pass the if-statement? > Sorry, I copied the wrong log line. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
On 2022-06-27 1:13 PM, Tao Liu wrote:
On Mon, Jun 27, 2022 at 11:51:48AM +0300, Roi Dayan wrote:
On 2022-06-27 10:49 AM, Tao Liu wrote:
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
---
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
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
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: 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 : parent :fff1 ingress_block 111
# ip l set net3 master bond0
# tc q ls dev net3 ingress
qdisc ingress : parent :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
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
On Mon, Jun 27, 2022 at 11:51:48AM +0300, Roi Dayan wrote:
>
>
> On 2022-06-27 10:49 AM, Tao Liu wrote:
> > 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
> > > > > > > > ---
> > > > > > > > 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
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
On 2022-06-27 10:49 AM, Tao Liu wrote:
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
---
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
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
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: 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 : parent :fff1 ingress_block 111
# ip l set net3 master bond0
# tc q ls dev net3 ingress
qdisc ingress : parent :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
you show a test for ovs adding ingress to slaves when bond0
is not part of ovs.
now with your patch also a
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
> > > > > > ---
> > > > > > 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
> > >
> >
> > 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
> >
>
> Hmm, sorry but looks like I did review too quick.
> I did some manual testing now and ovs doesn't ad
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
---
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
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
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?
+ 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
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
---
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
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
+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
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
> > > ---
> > > 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
>
> > > +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
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
> > ---
> > 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
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
> ---
> 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
[ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.
Bond master netdev may be created without a classification type, due
to routing or tunneling code.
If bond master is not attached to ovs, the ingress block on slaves shoud
not be updated.
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
---
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. */
+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
