On Mon, Mar 20, 2023 at 09:51:48AM +0200, Roi Dayan wrote: > > > On 14/03/2023 13:04, Simon Horman wrote: > > On Mon, Mar 13, 2023 at 07:47:14PM +0200, Roi Dayan wrote: > >> > >> > >> On 13/03/2023 14:16, Simon Horman wrote: > >>> On Mon, Mar 13, 2023 at 12:31:49PM +0200, Roi Dayan wrote: > >>>> > >>>> > >>>> On 13/03/2023 11:01, Eelco Chaudron wrote: > >>>>> > >>>>> > >>>>> On 13 Mar 2023, at 9:38, Roi Dayan wrote: > >>>>> > >>>>>> On 22/02/2023 12:30, Roi Dayan wrote: > >>>>>>> Sometimes there is a need to clean empty chains as done in > >>>>>>> delete_chains_from_netdev(). The cited commit doesn't remove > >>>>>>> the chain completely which cause adding ingress_block later to fail. > >>>>>>> This can be reproduced with adding bond as ovs port which makes ovs > >>>>>>> use ingress_block for it. > >>>>>>> While at it add the netdev name that fails to the log. > >>>>>>> > >>>>>>> Fixes: e1e5eac5b016 ("tc: Add TCA_KIND flower to delete and get > >>>>>>> operation to avoid rtnl_lock().") > >>>>>>> Signed-off-by: Roi Dayan <r...@nvidia.com> > >>>>>>> --- > >>>>>>> lib/netdev-offload-tc.c | 7 ++++--- > >>>>>>> lib/tc.c | 4 +++- > >>>>>>> 2 files changed, 7 insertions(+), 4 deletions(-) > >>>>>>> > >>>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > >>>>>>> index 4fb9d9f2127a..9dd0aa2e2a85 100644 > >>>>>>> --- a/lib/netdev-offload-tc.c > >>>>>>> +++ b/lib/netdev-offload-tc.c > >>>>>>> @@ -524,7 +524,7 @@ delete_chains_from_netdev(struct netdev *netdev, > >>>>>>> struct tcf_id *id) > >>>>>>> */ > >>>>>>> HMAP_FOR_EACH_POP (chain_node, node, &map) { > >>>>>>> id->chain = chain_node->chain; > >>>>>>> - tc_del_flower_filter(id); > >>>>>>> + tc_del_filter(id, NULL); > >>>>>>> free(chain_node); > >>>>>>> } > >>>>>>> } > >>>>>>> @@ -2860,8 +2860,9 @@ netdev_tc_init_flow_api(struct netdev *netdev) > >>>>>>> error = tc_add_del_qdisc(ifindex, true, block_id, hook); > >>>>>>> > >>>>>>> if (error && error != EEXIST) { > >>>>>>> - VLOG_INFO("failed adding ingress qdisc required for > >>>>>>> offloading: %s", > >>>>>>> - ovs_strerror(error)); > >>>>>>> + VLOG_INFO("failed adding ingress qdisc required for > >>>>>>> offloading " > >>>>>>> + "on %s: %s", > >>>>>>> + netdev_get_name(netdev), ovs_strerror(error)); > >>>>>>> return error; > >>>>>>> } > >>>>>>> > >>>>>>> diff --git a/lib/tc.c b/lib/tc.c > >>>>>>> index 4c07e22162e7..5c32c6f971da 100644 > >>>>>>> --- a/lib/tc.c > >>>>>>> +++ b/lib/tc.c > >>>>>>> @@ -2354,7 +2354,9 @@ tc_del_filter(struct tcf_id *id, const char > >>>>>>> *kind) > >>>>>>> struct ofpbuf request; > >>>>>>> > >>>>>>> request_from_tcf_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, &request); > >>>>>>> - nl_msg_put_string(&request, TCA_KIND, kind); > >>>>>>> + if (kind) { > >>>>>>> + nl_msg_put_string(&request, TCA_KIND, kind); > >>>>>>> + } > >>>>>>> return tc_transact(&request, NULL); > >>>>>>> } > >>>>>>> > >>>>>> > >>>>>> hi > >>>>>> > >>>>>> just pinging about this fix. > >>>>> > >>>>> Guess it’s waiting on your feedback on Simon’s reply: > >>>>> > >>>>> EC> The changes look good to me. Will it be worth adding a test case? > >>>>> > >>>>> SH> From my POV, yes, I think that would be nice. > >>>>> SH> Roi, do you have any thoughts on this? > >>>>> > >>>> > >>>> Oh. thanks for the updates. > >>>> I missed the replies. if I'm not on the to/cc the mailing list emails > >>>> are going to > >>>> a different folder so I could catch emails when I am on to/cc better. > >>> > >>> Sorry, I should have taken more care to CC you. > >>> I will try to do so in future. > >> > >> I'm having some trouble with adding a test for this. > >> > >> Internally I reproduce the issue with hw port with the following steps > >> > >> # ip l add dev bond0 type bond > >> # ip l set dev enp8s0f0 master bond0 > >> # ovs-vsctl add-port ov1 bond0 > >> # tc qdisc show dev bond0 > >> qdisc ingress ffff: parent ffff:fff1 ingress_block 563 ---------------- > >> # tc filter add block 563 ingress prio 1 flower action drop > >> # ovs-vsctl del-port ov1 bond0 > >> # ovs-vsctl add-port ov1 bond0 > >> # tc qdisc show dev bond0 > >> (no ingress_block) > >> > >> > >> Without adding a slave the issue doesn't happen and for the autoconf > >> test I wanted to use veth interface as a slave but the issue doesn't > >> reproduce with it as well. > >> > >> So we do need the fix as it solves us the issue but there is > >> something weird happening here. I'll try to look at this more > >> later this week or next. > > > > Thanks Roi, > > > > I understand. > > > > FWIIW, I am happy to move forwards with the fix if you follow-up with a > > test. > > > > Hi Simon, > > I'm still trying this between other stuff i need to do. > I couldn't reproduce this with veth. I'm not sure why or > what it means. I'm still trying every now and then. > I would be happy if we could still go with this fix to do > chains cleaning without related to kind flower as it does > help us and doesn't break anyone else.
Ilya, Eelco, all, are there any objections to taking this patch now. And allowing Roi to follow-up with a test later? _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev