| Message ID | 20240605161924.3162588-1-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3e61103b2f7887af0be402a79b9c70425ceba3e3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] page_pool: remove WARN_ON() with OR | expand |
On Wed, Jun 5, 2024 at 9:20 AM David Wei <dw@davidwei.uk> wrote: > > Having an OR in WARN_ON() makes me sad because it's impossible to tell > which condition is true when triggered. > > Split a WARN_ON() with an OR in page_pool_disable_direct_recycling(). > > Signed-off-by: David Wei <dw@davidwei.uk> Reviewed-by: Mina Almasry <almasrymina@google.com> > --- > net/core/page_pool.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index f4444b4e39e6..3927a0a7fa9a 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -1027,8 +1027,8 @@ static void page_pool_disable_direct_recycling(struct page_pool *pool) > /* To avoid races with recycling and additional barriers make sure > * pool and NAPI are unlinked when NAPI is disabled. > */ > - WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state) || > - READ_ONCE(pool->p.napi->list_owner) != -1); > + WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state)); > + WARN_ON(READ_ONCE(pool->p.napi->list_owner) != -1); > > WRITE_ONCE(pool->p.napi, NULL); > } > -- > 2.43.0 > >
On Wed, Jun 5, 2024 at 10:01 PM Mina Almasry <almasrymina@google.com> wrote: > > On Wed, Jun 5, 2024 at 9:20 AM David Wei <dw@davidwei.uk> wrote: > > > > Having an OR in WARN_ON() makes me sad because it's impossible to tell > > which condition is true when triggered. > > > > Split a WARN_ON() with an OR in page_pool_disable_direct_recycling(). > > > > Signed-off-by: David Wei <dw@davidwei.uk> > > Reviewed-by: Mina Almasry <almasrymina@google.com> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> > > --- > > net/core/page_pool.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > index f4444b4e39e6..3927a0a7fa9a 100644 > > --- a/net/core/page_pool.c > > +++ b/net/core/page_pool.c > > @@ -1027,8 +1027,8 @@ static void page_pool_disable_direct_recycling(struct page_pool *pool) > > /* To avoid races with recycling and additional barriers make sure > > * pool and NAPI are unlinked when NAPI is disabled. > > */ > > - WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state) || > > - READ_ONCE(pool->p.napi->list_owner) != -1); > > + WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state)); > > + WARN_ON(READ_ONCE(pool->p.napi->list_owner) != -1); > > > > WRITE_ONCE(pool->p.napi, NULL); > > } > > -- > > 2.43.0 > > > > > > > -- > Thanks, > Mina >
On 06/06/2024 10.58, Somnath Kotur wrote: > On Wed, Jun 5, 2024 at 10:01 PM Mina Almasry <almasrymina@google.com> wrote: >> >> On Wed, Jun 5, 2024 at 9:20 AM David Wei <dw@davidwei.uk> wrote: >>> >>> Having an OR in WARN_ON() makes me sad because it's impossible to tell >>> which condition is true when triggered. >>> >>> Split a WARN_ON() with an OR in page_pool_disable_direct_recycling(). >>> >>> Signed-off-by: David Wei <dw@davidwei.uk> >> >> Reviewed-by: Mina Almasry <almasrymina@google.com> >> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> LGTM Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> >>> --- >>> net/core/page_pool.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c >>> index f4444b4e39e6..3927a0a7fa9a 100644 >>> --- a/net/core/page_pool.c >>> +++ b/net/core/page_pool.c >>> @@ -1027,8 +1027,8 @@ static void page_pool_disable_direct_recycling(struct page_pool *pool) >>> /* To avoid races with recycling and additional barriers make sure >>> * pool and NAPI are unlinked when NAPI is disabled. >>> */ >>> - WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state) || >>> - READ_ONCE(pool->p.napi->list_owner) != -1); >>> + WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state)); >>> + WARN_ON(READ_ONCE(pool->p.napi->list_owner) != -1); >>> >>> WRITE_ONCE(pool->p.napi, NULL); >>> } >>> --
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 5 Jun 2024 09:19:24 -0700 you wrote: > Having an OR in WARN_ON() makes me sad because it's impossible to tell > which condition is true when triggered. > > Split a WARN_ON() with an OR in page_pool_disable_direct_recycling(). > > Signed-off-by: David Wei <dw@davidwei.uk> > > [...] Here is the summary with links: - [net-next,v1] page_pool: remove WARN_ON() with OR https://git.kernel.org/netdev/net-next/c/3e61103b2f78 You are awesome, thank you!
diff --git a/net/core/page_pool.c b/net/core/page_pool.c index f4444b4e39e6..3927a0a7fa9a 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -1027,8 +1027,8 @@ static void page_pool_disable_direct_recycling(struct page_pool *pool) /* To avoid races with recycling and additional barriers make sure * pool and NAPI are unlinked when NAPI is disabled. */ - WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state) || - READ_ONCE(pool->p.napi->list_owner) != -1); + WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state)); + WARN_ON(READ_ONCE(pool->p.napi->list_owner) != -1); WRITE_ONCE(pool->p.napi, NULL); }
Having an OR in WARN_ON() makes me sad because it's impossible to tell which condition is true when triggered. Split a WARN_ON() with an OR in page_pool_disable_direct_recycling(). Signed-off-by: David Wei <dw@davidwei.uk> --- net/core/page_pool.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)