On 25/04/2023 18:56, Ilya Maximets wrote: > On 4/25/23 08:30, Roi Dayan wrote: >> >> >> On 20/03/2023 13:49, Eelco Chaudron wrote: >>> >>> >>> On 20 Mar 2023, at 11:48, Ilya Maximets wrote: >>> >>>> On 3/20/23 11:44, Simon Horman wrote: >>>>> 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? >>>>> >>>> >>>> Fine by me. Though it's a bit concerning that the issue is not >>>> reproducible. Maybe we should update the comment in the code >>>> stating why we need to remove not only flower chains? To avoid >>>> messing up this part in the future again. >>> >>> +1 >>> >> >> Hi, >> >> So if we can get this in that would be great. > > Could you re-spin the patch adding the comment to the code on > "why tc_del_filter is used instead of tc_del_flower_filter?" > in the delete_chains_from_netdev() ? > > It's unclear from the code why it is the case and might lead to > repeating the mistake in the future, especially since we do not > have a test covering that case. > > Best regards, Ilya Maximets.
sure. I sent v2 with an added comment. Thanks, Roi _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev