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