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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
