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 <[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
you show a test for ovs adding ingress to slaves when bond0
is not part of ovs.
now with your patch also adding bond0 to ovs bridge doesn't add the
ingress to the bond slaves. only to bond0 itself.
+ 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
