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