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
