diff mbox series

[v4,1/2] net: page_pool: export page_pool_stats definition

Message ID 20221115155744.193789-2-shenwei.wang@nxp.com
State New
Headers
Series net: fec: add xdp and page pool statistics |

Commit Message

Shenwei Wang Nov. 15, 2022, 3:57 p.m. UTC 
  The definition of the 'struct page_pool_stats' is required even when
the CONFIG_PAGE_POOL_STATS is not defined. Otherwise, it is required
the drivers to handle the case of CONFIG_PAGE_POOL_STATS undefined.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 include/net/page_pool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Lunn Nov. 15, 2022, 5:12 p.m. UTC | #1 
On Tue, Nov 15, 2022 at 09:57:43AM -0600, Shenwei Wang wrote:
> The definition of the 'struct page_pool_stats' is required even when
> the CONFIG_PAGE_POOL_STATS is not defined. Otherwise, it is required
> the drivers to handle the case of CONFIG_PAGE_POOL_STATS undefined.

I agree the API is broken, but i think there is a better fix.

There should be a stub of page_pool_get_stats() for when
CONFIG_PAGE_POOL_STATS is disabled.

Nothing actually dereferences struct page_pool_stats when you have
this stub. So it might be enough to simply have

struct page_pool_stats{
};

       Andrew
Shenwei Wang Nov. 15, 2022, 5:18 p.m. UTC | #2 
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, November 15, 2022 11:12 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Jesper Dangaard Brouer <hawk@kernel.org>; Ilias
> Apalodimas <ilias.apalodimas@linaro.org>; Alexei Starovoitov
> <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; John Fastabend
> <john.fastabend@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH v4 1/2] net: page_pool: export page_pool_stats
> definition
> 
> Caution: EXT Email
> 
> On Tue, Nov 15, 2022 at 09:57:43AM -0600, Shenwei Wang wrote:
> > The definition of the 'struct page_pool_stats' is required even when
> > the CONFIG_PAGE_POOL_STATS is not defined. Otherwise, it is required
> > the drivers to handle the case of CONFIG_PAGE_POOL_STATS undefined.
> 
> I agree the API is broken, but i think there is a better fix.
> 
> There should be a stub of page_pool_get_stats() for when
> CONFIG_PAGE_POOL_STATS is disabled.
> 
> Nothing actually dereferences struct page_pool_stats when you have this stub.
> So it might be enough to simply have
> 
> struct page_pool_stats{
> };
> 

As the structure is open when the CONFIG_PAGE_POOL_STATS is enabled, you can not
prevent a user to access its members. The empty stub will still have problems in this
kind of situations.

Regards,
Shenwei

>        Andrew
Andrew Lunn Nov. 15, 2022, 5:43 p.m. UTC | #3 
> > I agree the API is broken, but i think there is a better fix.
> > 
> > There should be a stub of page_pool_get_stats() for when
> > CONFIG_PAGE_POOL_STATS is disabled.
> > 
> > Nothing actually dereferences struct page_pool_stats when you have this stub.
> > So it might be enough to simply have
> > 
> > struct page_pool_stats{
> > };
> > 
> 
> As the structure is open when the CONFIG_PAGE_POOL_STATS is enabled, you can not
> prevent a user to access its members. The empty stub will still have problems in this
> kind of situations.

The users, i.e. the driver, has no need to access its members. The
members can change, new ones can be added, and it will not cause a
problem, given the way this API is defined. Ideally, page_pool_stats
would of been an opaque cookie, but that is not how it was
implemented :-(

	    Andrew
Shenwei Wang Nov. 15, 2022, 5:52 p.m. UTC | #4 
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, November 15, 2022 11:43 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Jesper Dangaard Brouer <hawk@kernel.org>; Ilias
> Apalodimas <ilias.apalodimas@linaro.org>; Alexei Starovoitov
> <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; John Fastabend
> <john.fastabend@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [EXT] Re: [PATCH v4 1/2] net: page_pool: export page_pool_stats
> definition
> 
> Caution: EXT Email
> 
> > > I agree the API is broken, but i think there is a better fix.
> > >
> > > There should be a stub of page_pool_get_stats() for when
> > > CONFIG_PAGE_POOL_STATS is disabled.
> > >
> > > Nothing actually dereferences struct page_pool_stats when you have this
> stub.
> > > So it might be enough to simply have
> > >
> > > struct page_pool_stats{
> > > };
> > >
> >
> > As the structure is open when the CONFIG_PAGE_POOL_STATS is enabled,
> > you can not prevent a user to access its members. The empty stub will
> > still have problems in this kind of situations.
> 
> The users, i.e. the driver, has no need to access its members. The members can
> change, new ones can be added, and it will not cause a problem, given the way
> this API is defined. Ideally, page_pool_stats would of been an opaque cookie,
> but that is not how it was implemented :-(
> 

Okay if the assumption is the user would never access its members.

Regards,
Shenwei

>             Andrew
diff mbox series

Patch

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 813c93499f20..adfd4bb609a7 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -84,7 +84,6 @@  struct page_pool_params {
 	void *init_arg;
 };
 
-#ifdef CONFIG_PAGE_POOL_STATS
 struct page_pool_alloc_stats {
 	u64 fast; /* fast path allocations */
 	u64 slow; /* slow-path order 0 allocations */
@@ -117,6 +116,7 @@  struct page_pool_stats {
 	struct page_pool_recycle_stats recycle_stats;
 };
 
+#ifdef CONFIG_PAGE_POOL_STATS
 int page_pool_ethtool_stats_get_count(void);
 u8 *page_pool_ethtool_stats_get_strings(u8 *data);
 u64 *page_pool_ethtool_stats_get(u64 *data, void *stats);