Message-ID: <48e9bf0a-3275-4d2c-84ae-9bc1163ab8cb@fiberby.net>
Date: Fri, 10 Jan 2025 13:25:18 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: Xin Long <lucien.xin@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Jamal Hadi Salim <jhs@...atatu.com>,
 Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
 Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
 Shuang Li <shuali@...hat.com>, network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: sched: refine software bypass handling in tc_run

Sorry for the late response, it has been a busy first week back, too many
operational issues with devices in our network running crappy vendor images.

On 1/6/25 3:08 PM, Xin Long wrote:
> This patch addresses issues with filter counting in block (tcf_block),
> particularly for software bypass scenarios, by introducing a more
> accurate mechanism using useswcnt.
> 
> Previously, filtercnt and skipswcnt were introduced by:
> 
>    Commit 2081fd3445fe ("net: sched: cls_api: add filter counter") and
>    Commit f631ef39d819 ("net: sched: cls_api: add skip_sw counter")
> 
>    filtercnt tracked all tp (tcf_proto) objects added to a block, and
>    skipswcnt counted tp objects with the skipsw attribute set.
> 
> The problem is: a single tp can contain multiple filters, some with skipsw
> and others without. The current implementation fails in the case:
> 
>    When the first filter in a tp has skipsw, both skipswcnt and filtercnt
>    are incremented, then adding a second filter without skipsw to the same
>    tp does not modify these counters because tp->counted is already set.
> 
>    This results in bypass software behavior based solely on skipswcnt
>    equaling filtercnt, even when the block includes filters without
>    skipsw. Consequently, filters without skipsw are inadvertently bypassed.

Thank you for tracking it down. I wasn't aware that a tp, could be used by multiple
filters, and didn't encounter it during my testing.

> To address this, the patch introduces useswcnt in block to explicitly count
> tp objects containing at least one filter without skipsw. Key changes
> include:
> 
>    Whenever a filter without skipsw is added, its tp is marked with usesw
>    and counted in useswcnt. tc_run() now uses useswcnt to determine software
>    bypass, eliminating reliance on filtercnt and skipswcnt.
> 
>    This refined approach prevents software bypass for blocks containing
>    mixed filters, ensuring correct behavior in tc_run().
> 
> Additionally, as atomic operations on useswcnt ensure thread safety and
> tp->lock guards access to tp->usesw and tp->counted, the broader lock
> down_write(&block->cb_lock) is no longer required in tc_new_tfilter(),
> and this resolves a performance regression caused by the filter counting
> mechanism during parallel filter insertions.

You are trying to do two things:
A) Fix functional defect when filters share a single tp
B) Improve filter updates performance

If you do part A in a minimalistic way, then IMHO it might be suitable
for net (+ stable), but for part B I agree with Paolo, that it would
properly be better suited for net-next.

I focused my testing on routing performance, not filter update performance,
I also didn't test it in any multi-CPU setups (as I don't have any).

The static key was added to mitigate concerns, about the impact that the
bypass check would have for non-offloaded workloads in multi-CPU systems.

https://lore.kernel.org/netdev/28bf1467-b7ce-4e36-a4ef-5445f65edd97@fiberby.net/
https://lore.kernel.org/netdev/CAM0EoMngVoBcbX7cqTdbW8dG1v_ysc1SZK+4y-9j-5Tbq6gaYw@mail.gmail.com/

Powered by blists - more mailing lists