Re: [PATCH net] net/sched: add delete_empty() to filters and use it in cls_flower

[Vlad Buslov <vladbu@mellanox.com>]

On Sat 28 Dec 2019 at 17:36, Davide Caratti <dcaratti@redhat.com> wrote:
> Revert "net/sched: cls_u32: fix refcount leak in the error path of
> u32_change()", and fix the u32 refcount leak in a more generic way that
> preserves the semantic of rule dumping.
> On tc filters that don't support lockless insertion/removal, there is no
> need to guard against concurrent insertion when a removal is in progress.
> Therefore, for most of them we can avoid a full walk() when deleting, and
> just decrease the refcount, like it was done on older Linux kernels.
> This fixes situations where walk() was wrongly detecting a non-empty
> filter, like it happened with cls_u32 in the error path of change(), thus
> leading to failures in the following tdc selftests:
>
>  6aa7: (filter, u32) Add/Replace u32 with source match and invalid indev
>  6658: (filter, u32) Add/Replace u32 with custom hash table and invalid handle
>  74c2: (filter, u32) Add/Replace u32 filter with invalid hash table id
>
> On cls_flower, and on (future) lockless filters, this check is necessary:
> move all the check_empty() logic in a callback so that each filter
> can have its own implementation. For cls_flower, it's sufficient to check
> if no IDRs have been allocated.
>
> This reverts commit 275c44aa194b7159d1191817b20e076f55f0e620.
>
> Changes since v1:
>  - document the need for delete_empty() when TCF_PROTO_OPS_DOIT_UNLOCKED
>    is used, thanks to Vlad Buslov
>  - implement delete_empty() without doing fl_walk(), thanks to Vlad Buslov
>  - squash revert and new fix in a single patch, to be nice with bisect
>    tests that run tdc on u32 filter, thanks to Dave Miller
>
> Fixes: 275c44aa194b ("net/sched: cls_u32: fix refcount leak in the error path of u32_change()")
> Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Suggested-by: Vlad Buslov <vladbu@mellanox.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---

Thanks again, Davide!

Reviewed-by: Vlad Buslov <vladbu@mellanox.com>