diff mbox series

[net-next,5/9] page_pool: don't use driver-set flags field directly

Message ID 20230727144336.1646454-6-aleksander.lobakin@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series page_pool: a couple of assorted optimizations | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1412 this patch: 1412
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1371 this patch: 1371
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1444 this patch: 1444
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 139 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alexander Lobakin July 27, 2023, 2:43 p.m. UTC
page_pool::p is driver-defined params, copied directly from the
structure passed via page_pool_create(). The structure isn't meant
to be modified by the Page Pool core code and this even might look
confusing[0][1].
In order to be able to alter some flags, let's define our own, internal
fields. Use the slot freed earlier to stay within the same cacheline as
before (or almost if it's shorter than 64 bytes).
The flags indicating whether to perform DMA mapping and use frags can
be bool; as for DMA sync, define it as an enum to be able to extend it
later on. They are defined as bits in the driver-set params, leave them
so here as well, to not waste byte-per-bit or so. Now there are 29 free
bits left in those 4 bytes + 4 free bytes more before the cacheline
boundary.
We could've defined only new flags here or only the ones we may need
to alter, but checking some flags in one place while others in another
doesn't sound convenient or intuitive.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Link[0]: https://lore.kernel.org/netdev/20230703133207.4f0c54ce@kernel.org
Suggested-by: Alexander Duyck <alexanderduyck@fb.com>
Link[1]: https://lore.kernel.org/netdev/CAKgT0UfZCGnWgOH96E4GV3ZP6LLbROHM7SHE8NKwq+exX+Gk_Q@mail.gmail.com
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 include/net/page_pool/helpers.h |  2 +-
 include/net/page_pool/types.h   |  8 +++++++-
 net/core/page_pool.c            | 33 +++++++++++++++++----------------
 3 files changed, 25 insertions(+), 18 deletions(-)

Comments

Yunsheng Lin July 28, 2023, 12:36 p.m. UTC | #1
On 2023/7/27 22:43, Alexander Lobakin wrote:

>  
>  struct page_pool {
>  	struct page_pool_params p;
> -	long pad;
> +
> +	bool dma_map:1;				/* Perform DMA mapping */
> +	enum {
> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
> +	} dma_sync_act:1;
> +	bool page_frag:1;			/* Allow page fragments */
>  

Isn't it more common or better to just remove the flags field in
'struct page_pool_params' and pass the flags by parameter like
below, so that patch 4 is not needed?

struct page_pool *page_pool_create(const struct page_pool_params *params,
				   unsigned int	flags);
Alexander Lobakin July 28, 2023, 2:03 p.m. UTC | #2
From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Fri, 28 Jul 2023 20:36:50 +0800

> On 2023/7/27 22:43, Alexander Lobakin wrote:
> 
>>  
>>  struct page_pool {
>>  	struct page_pool_params p;
>> -	long pad;
>> +
>> +	bool dma_map:1;				/* Perform DMA mapping */
>> +	enum {
>> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
>> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
>> +	} dma_sync_act:1;
>> +	bool page_frag:1;			/* Allow page fragments */
>>  
> 
> Isn't it more common or better to just remove the flags field in
> 'struct page_pool_params' and pass the flags by parameter like
> below, so that patch 4 is not needed?
> 
> struct page_pool *page_pool_create(const struct page_pool_params *params,
> 				   unsigned int	flags);

You would need a separate patch to convert all the page_pool_create()
users then either way.
And it doesn't look really natural to me to pass both driver-set params
and driver-set flags as separate function arguments. Someone may then
think "why aren't flags just put in the params itself". The fact that
Page Pool copies the whole params in the page_pool struct after
allocating it is internals, page_pool_create() prototype however isn't.
Thoughts?

Thanks,
Olek
Jakub Kicinski July 28, 2023, 3:50 p.m. UTC | #3
On Fri, 28 Jul 2023 16:03:53 +0200 Alexander Lobakin wrote:
> And it doesn't look really natural to me to pass both driver-set params
> and driver-set flags as separate function arguments.

+1
Yunsheng Lin July 29, 2023, 11:40 a.m. UTC | #4
On 2023/7/28 22:03, Alexander Lobakin wrote:
> From: Yunsheng Lin <linyunsheng@huawei.com>
> Date: Fri, 28 Jul 2023 20:36:50 +0800
> 
>> On 2023/7/27 22:43, Alexander Lobakin wrote:
>>
>>>  
>>>  struct page_pool {
>>>  	struct page_pool_params p;
>>> -	long pad;
>>> +
>>> +	bool dma_map:1;				/* Perform DMA mapping */
>>> +	enum {
>>> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
>>> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
>>> +	} dma_sync_act:1;
>>> +	bool page_frag:1;			/* Allow page fragments */
>>>  
>>
>> Isn't it more common or better to just remove the flags field in
>> 'struct page_pool_params' and pass the flags by parameter like
>> below, so that patch 4 is not needed?
>>
>> struct page_pool *page_pool_create(const struct page_pool_params *params,
>> 				   unsigned int	flags);
> 
> You would need a separate patch to convert all the page_pool_create()
> users then either way.
> And it doesn't look really natural to me to pass both driver-set params
> and driver-set flags as separate function arguments. Someone may then
> think "why aren't flags just put in the params itself". The fact that
> Page Pool copies the whole params in the page_pool struct after
> allocating it is internals, page_pool_create() prototype however isn't.
> Thoughts?

It just seems odd to me that dma_map and page_frag is duplicated as we
seems to have the same info in the page_pool->p.flags.

What about:
In [PATCH net-next 4/9] page_pool: shrink &page_pool_params a tiny bit,
'flags' is bit-field'ed with 'dma_dir', what about changing 'dma_dir'
to be bit-field'ed with 'dma_sync_act', so that page_pool->p.flags stays
the same as before, and 'dma_map' & 'page_frag' do not seems be really
needed as we have the same info in page_pool->p.flags?


> 
> Thanks,
> Olek
> 
> .
>
Alexander Lobakin Aug. 1, 2023, 1:36 p.m. UTC | #5
From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Sat, 29 Jul 2023 19:40:32 +0800

> On 2023/7/28 22:03, Alexander Lobakin wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Fri, 28 Jul 2023 20:36:50 +0800
>>
>>> On 2023/7/27 22:43, Alexander Lobakin wrote:
>>>
>>>>  
>>>>  struct page_pool {
>>>>  	struct page_pool_params p;
>>>> -	long pad;
>>>> +
>>>> +	bool dma_map:1;				/* Perform DMA mapping */
>>>> +	enum {
>>>> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
>>>> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
>>>> +	} dma_sync_act:1;
>>>> +	bool page_frag:1;			/* Allow page fragments */
>>>>  
>>>
>>> Isn't it more common or better to just remove the flags field in
>>> 'struct page_pool_params' and pass the flags by parameter like
>>> below, so that patch 4 is not needed?
>>>
>>> struct page_pool *page_pool_create(const struct page_pool_params *params,
>>> 				   unsigned int	flags);
>>
>> You would need a separate patch to convert all the page_pool_create()
>> users then either way.
>> And it doesn't look really natural to me to pass both driver-set params
>> and driver-set flags as separate function arguments. Someone may then
>> think "why aren't flags just put in the params itself". The fact that
>> Page Pool copies the whole params in the page_pool struct after
>> allocating it is internals, page_pool_create() prototype however isn't.
>> Thoughts?
> 
> It just seems odd to me that dma_map and page_frag is duplicated as we
> seems to have the same info in the page_pool->p.flags.

It's just because we copy the whole &page_pool_params passed by the
driver. It doesn't look good to me to define a new structure and copy
the values field-by-field just to avoid duplicating 3 bits :s

> 
> What about:
> In [PATCH net-next 4/9] page_pool: shrink &page_pool_params a tiny bit,
> 'flags' is bit-field'ed with 'dma_dir', what about changing 'dma_dir'
> to be bit-field'ed with 'dma_sync_act', so that page_pool->p.flags stays
> the same as before, and 'dma_map' & 'page_frag' do not seems be really
> needed as we have the same info in page_pool->p.flags?

Not sure I follow :z ::dma_dir is also passed by the driver, so we can't
drop it from the params struct.

> 
> 
>>
>> Thanks,
>> Olek
>>
>> .
>>

Thanks,
Olek
Yunsheng Lin Aug. 2, 2023, 11:36 a.m. UTC | #6
On 2023/8/1 21:36, Alexander Lobakin wrote:

>>
>> It just seems odd to me that dma_map and page_frag is duplicated as we
>> seems to have the same info in the page_pool->p.flags.
> 
> It's just because we copy the whole &page_pool_params passed by the
> driver. It doesn't look good to me to define a new structure and copy
> the values field-by-field just to avoid duplicating 3 bits :s
> 
>>
>> What about:
>> In [PATCH net-next 4/9] page_pool: shrink &page_pool_params a tiny bit,
>> 'flags' is bit-field'ed with 'dma_dir', what about changing 'dma_dir'
>> to be bit-field'ed with 'dma_sync_act', so that page_pool->p.flags stays
>> the same as before, and 'dma_map' & 'page_frag' do not seems be really
>> needed as we have the same info in page_pool->p.flags?
> 
> Not sure I follow :z ::dma_dir is also passed by the driver, so we can't
> drop it from the params struct.

My bad, I missed that dma_dir is in the params struct.
Jakub Kicinski Aug. 2, 2023, 9:29 p.m. UTC | #7
On Tue, 1 Aug 2023 15:36:33 +0200 Alexander Lobakin wrote:
> >> You would need a separate patch to convert all the page_pool_create()
> >> users then either way.
> >> And it doesn't look really natural to me to pass both driver-set params
> >> and driver-set flags as separate function arguments. Someone may then
> >> think "why aren't flags just put in the params itself". The fact that
> >> Page Pool copies the whole params in the page_pool struct after
> >> allocating it is internals, page_pool_create() prototype however isn't.
> >> Thoughts?  
> > 
> > It just seems odd to me that dma_map and page_frag is duplicated as we
> > seems to have the same info in the page_pool->p.flags.  
> 
> It's just because we copy the whole &page_pool_params passed by the
> driver. It doesn't look good to me to define a new structure and copy
> the values field-by-field just to avoid duplicating 3 bits :s

FWIW I'm tempted to do something like the patch below (an obvious move,
I suspect). I want to add another pointer (netdev) to the params and 
I don't want it to eat up bytes in the first cache line.
The patch is incomplete, we need to stash a one-bit indication in 
the first cache line to know init_callback is not present without
having to look at @slow. I'll defer doing that cleanly until your
patches land.
With this in place we can move flags outside of @fast, and interpret
it manually while copying all the other members in one go.

--->8-------------------------------

From c1290e74c3ec54090a49d0c88ca9d56c3bede825 Mon Sep 17 00:00:00 2001
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 2 Aug 2023 14:16:51 -0700
Subject: [PATCH] net: page_pool: split the page_pool_params into fast and slow

struct page_pool is rather performance critical and we use
16B of the first cache line to store 2 pointers used only
by test code. Future patches will add more informational
(non-fast path) attributes.

It's convenient for the user of the API to not have to worry
which fields are fast and which are slow path. Use struct
groups to split the params into the two categories internally.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/page_pool.h | 31 +++++++++++++++++++------------
 net/core/page_pool.c    |  7 ++++---
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 73d4f786418d..f0267279a8cd 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -83,18 +83,22 @@ struct pp_alloc_cache {
  * @offset:	DMA sync address offset for PP_FLAG_DMA_SYNC_DEV
  */
 struct page_pool_params {
-	unsigned int	flags;
-	unsigned int	order;
-	unsigned int	pool_size;
-	int		nid;
-	struct device	*dev;
-	struct napi_struct *napi;
-	enum dma_data_direction dma_dir;
-	unsigned int	max_len;
-	unsigned int	offset;
+	struct_group_tagged(page_pool_params_fast, fast,
+		unsigned int	flags;
+		unsigned int	order;
+		unsigned int	pool_size;
+		int		nid;
+		struct device	*dev;
+		struct napi_struct *napi;
+		enum dma_data_direction dma_dir;
+		unsigned int	max_len;
+		unsigned int	offset;
+	);
+	struct_group_tagged(page_pool_params_slow, slow,
 /* private: used by test code only */
-	void (*init_callback)(struct page *page, void *arg);
-	void *init_arg;
+		void (*init_callback)(struct page *page, void *arg);
+		void *init_arg;
+	);
 };
 
 #ifdef CONFIG_PAGE_POOL_STATS
@@ -177,7 +181,7 @@ static inline u64 *page_pool_ethtool_stats_get(u64 *data, void *stats)
 #endif
 
 struct page_pool {
-	struct page_pool_params p;
+	struct page_pool_params_fast p;
 
 	struct delayed_work release_dw;
 	void (*disconnect)(void *);
@@ -236,6 +240,9 @@ struct page_pool {
 	refcount_t user_cnt;
 
 	u64 destroy_cnt;
+
+	/* Slow/Control-path information follows */
+	struct page_pool_params_slow slow;
 };
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5d615a169718..fc3f6878a002 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -173,7 +173,8 @@ static int page_pool_init(struct page_pool *pool,
 {
 	unsigned int ring_qsize = 1024; /* Default */
 
-	memcpy(&pool->p, params, sizeof(pool->p));
+	memcpy(&pool->p, &params->fast, sizeof(pool->p));
+	memcpy(&pool->slow, &params->slow, sizeof(pool->slow));
 
 	/* Validate only known flags were used */
 	if (pool->p.flags & ~(PP_FLAG_ALL))
@@ -372,8 +373,8 @@ static void page_pool_set_pp_info(struct page_pool *pool,
 {
 	page->pp = pool;
 	page->pp_magic |= PP_SIGNATURE;
-	if (pool->p.init_callback)
-		pool->p.init_callback(page, pool->p.init_arg);
+	if (pool->slow.init_callback)
+		pool->slow.init_callback(page, pool->slow.init_arg);
 }
 
 static void page_pool_clear_pp_info(struct page *page)
Alexander Lobakin Aug. 3, 2023, 2:56 p.m. UTC | #8
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 2 Aug 2023 14:29:20 -0700

> On Tue, 1 Aug 2023 15:36:33 +0200 Alexander Lobakin wrote:
>>>> You would need a separate patch to convert all the page_pool_create()
>>>> users then either way.
>>>> And it doesn't look really natural to me to pass both driver-set params
>>>> and driver-set flags as separate function arguments. Someone may then
>>>> think "why aren't flags just put in the params itself". The fact that
>>>> Page Pool copies the whole params in the page_pool struct after
>>>> allocating it is internals, page_pool_create() prototype however isn't.
>>>> Thoughts?  
>>>
>>> It just seems odd to me that dma_map and page_frag is duplicated as we
>>> seems to have the same info in the page_pool->p.flags.  
>>
>> It's just because we copy the whole &page_pool_params passed by the
>> driver. It doesn't look good to me to define a new structure and copy
>> the values field-by-field just to avoid duplicating 3 bits :s
> 
> FWIW I'm tempted to do something like the patch below (an obvious move,
> I suspect). I want to add another pointer (netdev) to the params and 

Just take napi->dev as I do in libie :)

> I don't want it to eat up bytes in the first cache line.
> The patch is incomplete, we need to stash a one-bit indication in 
> the first cache line to know init_callback is not present without
> having to look at @slow. I'll defer doing that cleanly until your
> patches land.

I would propose to include it in the series, but it has grown a bunch
already and it's better to do that later separately :s

> With this in place we can move flags outside of @fast, and interpret

Oh, really nice. We could avoid copying them at all.

> it manually while copying all the other members in one go.

[...]

Thanks,
Olek
Jakub Kicinski Aug. 3, 2023, 4 p.m. UTC | #9
On Thu, 3 Aug 2023 16:56:22 +0200 Alexander Lobakin wrote:
> > FWIW I'm tempted to do something like the patch below (an obvious move,
> > I suspect). I want to add another pointer (netdev) to the params and   
> 
> Just take napi->dev as I do in libie :)

:) The fields have extra semantics, like napi implies that recycling 
is allowed, and netdev implies that there is only _one_ netdev eating
from the PP. There's also a way to get the pp <> netdev from the memory
model registration. But I feel like explicit field is cleanest.

Anyone, conversation for a later time :)

> > I don't want it to eat up bytes in the first cache line.
> > The patch is incomplete, we need to stash a one-bit indication in 
> > the first cache line to know init_callback is not present without
> > having to look at @slow. I'll defer doing that cleanly until your
> > patches land.  
> 
> I would propose to include it in the series, but it has grown a bunch
> already and it's better to do that later separately :s

Yeah.. I'd be trying to split your series up a little to make progress
rather than add more things :( I was going to suggest that you post
just the first 3 patches for instance. Should be an easy merge.
Alexander Lobakin Aug. 3, 2023, 4:07 p.m. UTC | #10
From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 3 Aug 2023 09:00:29 -0700

> On Thu, 3 Aug 2023 16:56:22 +0200 Alexander Lobakin wrote:
>>> FWIW I'm tempted to do something like the patch below (an obvious move,
>>> I suspect). I want to add another pointer (netdev) to the params and 

[...]

>> I would propose to include it in the series, but it has grown a bunch
>> already and it's better to do that later separately :s
> 
> Yeah.. I'd be trying to split your series up a little to make progress
> rather than add more things :( I was going to suggest that you post
> just the first 3 patches for instance. Should be an easy merge.

One minute before I was going to post v2 :>
Sounds good. AFACS only #4-6 are still under question (not for me tho
:D), let me move that piece out.

Thanks,
Olek
Jakub Kicinski Aug. 3, 2023, 4:40 p.m. UTC | #11
On Thu, 3 Aug 2023 18:07:23 +0200 Alexander Lobakin wrote:
> >> I would propose to include it in the series, but it has grown a bunch
> >> already and it's better to do that later separately :s  
> > 
> > Yeah.. I'd be trying to split your series up a little to make progress
> > rather than add more things :( I was going to suggest that you post
> > just the first 3 patches for instance. Should be an easy merge.  
> 
> One minute before I was going to post v2 :>
> Sounds good. AFACS only #4-6 are still under question (not for me tho
> :D), let me move that piece out.

FWIW I'll apply my doc changes in the next 5min if nobody objects.
Can you rebase on top of that?
Alexander Lobakin Aug. 3, 2023, 4:42 p.m. UTC | #12
From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 3 Aug 2023 09:40:43 -0700

> On Thu, 3 Aug 2023 18:07:23 +0200 Alexander Lobakin wrote:
>>>> I would propose to include it in the series, but it has grown a bunch
>>>> already and it's better to do that later separately :s  
>>>
>>> Yeah.. I'd be trying to split your series up a little to make progress
>>> rather than add more things :( I was going to suggest that you post
>>> just the first 3 patches for instance. Should be an easy merge.  
>>
>> One minute before I was going to post v2 :>
>> Sounds good. AFACS only #4-6 are still under question (not for me tho
>> :D), let me move that piece out.
> 
> FWIW I'll apply my doc changes in the next 5min if nobody objects.
> Can you rebase on top of that?

Breh, sent already >_<

What do I do then? Rebase and send v3?

Thanks,
Olek
diff mbox series

Patch

diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h
index e2d8d3a8810c..a09ba80b889e 100644
--- a/include/net/page_pool/helpers.h
+++ b/include/net/page_pool/helpers.h
@@ -125,7 +125,7 @@  static inline bool page_pool_is_last_frag(struct page_pool *pool,
 					  struct page *page)
 {
 	/* If fragments aren't enabled or count is 0 we were the last user */
-	return !(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
+	return !pool->page_frag ||
 	       (page_pool_defrag_page(page, 1) == 0);
 }
 
diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index c86f65e57614..dd26f4b2b66c 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -93,7 +93,13 @@  struct page_pool_stats {
 
 struct page_pool {
 	struct page_pool_params p;
-	long pad;
+
+	bool dma_map:1;				/* Perform DMA mapping */
+	enum {
+		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
+		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
+	} dma_sync_act:1;
+	bool page_frag:1;			/* Allow page fragments */
 
 	long frag_users;
 	struct page *frag_page;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 7a23ca6b1124..6a8f105e2df5 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -183,6 +183,8 @@  static int page_pool_init(struct page_pool *pool,
 		if ((pool->p.dma_dir != DMA_FROM_DEVICE) &&
 		    (pool->p.dma_dir != DMA_BIDIRECTIONAL))
 			return -EINVAL;
+
+		pool->dma_map = true;
 	}
 
 	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV) {
@@ -195,13 +197,15 @@  static int page_pool_init(struct page_pool *pool,
 		if (!pool->p.max_len)
 			return -EINVAL;
 
+		pool->dma_sync_act = PP_DMA_SYNC_ACT_DO;
+
 		/* pool->p.offset has to be set according to the address
 		 * offset used by the DMA engine to start copying rx data
 		 */
 	}
 
-	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
-	    pool->p.flags & PP_FLAG_PAGE_FRAG)
+	pool->page_frag = !!(pool->p.flags & PP_FLAG_PAGE_FRAG);
+	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT && pool->page_frag)
 		return -EINVAL;
 
 #ifdef CONFIG_PAGE_POOL_STATS
@@ -218,7 +222,7 @@  static int page_pool_init(struct page_pool *pool,
 	/* Driver calling page_pool_create() also call page_pool_destroy() */
 	refcount_set(&pool->user_cnt, 1);
 
-	if (pool->p.flags & PP_FLAG_DMA_MAP)
+	if (pool->dma_map)
 		get_device(pool->p.dev);
 
 	return 0;
@@ -346,7 +350,7 @@  static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
 
 	page_pool_set_dma_addr(page, dma);
 
-	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+	if (pool->dma_sync_act == PP_DMA_SYNC_ACT_DO)
 		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
 
 	return true;
@@ -377,8 +381,7 @@  static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
 	if (unlikely(!page))
 		return NULL;
 
-	if ((pool->p.flags & PP_FLAG_DMA_MAP) &&
-	    unlikely(!page_pool_dma_map(pool, page))) {
+	if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page))) {
 		put_page(page);
 		return NULL;
 	}
@@ -398,8 +401,8 @@  static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 						 gfp_t gfp)
 {
 	const int bulk = PP_ALLOC_CACHE_REFILL;
-	unsigned int pp_flags = pool->p.flags;
 	unsigned int pp_order = pool->p.order;
+	bool dma_map = pool->dma_map;
 	struct page *page;
 	int i, nr_pages;
 
@@ -424,8 +427,7 @@  static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	 */
 	for (i = 0; i < nr_pages; i++) {
 		page = pool->alloc.cache[i];
-		if ((pp_flags & PP_FLAG_DMA_MAP) &&
-		    unlikely(!page_pool_dma_map(pool, page))) {
+		if (dma_map && unlikely(!page_pool_dma_map(pool, page))) {
 			put_page(page);
 			continue;
 		}
@@ -497,7 +499,7 @@  static void page_pool_return_page(struct page_pool *pool, struct page *page)
 	dma_addr_t dma;
 	int count;
 
-	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
+	if (!pool->dma_map)
 		/* Always account for inflight pages, even if we didn't
 		 * map them
 		 */
@@ -563,7 +565,7 @@  static bool page_pool_recycle_in_cache(struct page *page,
 }
 
 /* If the page refcnt == 1, this will try to recycle the page.
- * if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
+ * if pool->dma_sync_act is set, we'll try to sync the DMA area for
  * the configured size min(dma_sync_size, pool->max_len).
  * If the page refcnt != 1, then the page will be returned to memory
  * subsystem.
@@ -584,7 +586,7 @@  __page_pool_put_page(struct page_pool *pool, struct page *page,
 	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
-		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+		if (pool->dma_sync_act == PP_DMA_SYNC_ACT_DO)
 			page_pool_dma_sync_for_device(pool, page,
 						      dma_sync_size);
 
@@ -683,7 +685,7 @@  static struct page *page_pool_drain_frag(struct page_pool *pool,
 		return NULL;
 
 	if (page_ref_count(page) == 1 && !page_is_pfmemalloc(page)) {
-		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+		if (pool->dma_sync_act == PP_DMA_SYNC_ACT_DO)
 			page_pool_dma_sync_for_device(pool, page, -1);
 
 		return page;
@@ -713,8 +715,7 @@  struct page *page_pool_alloc_frag(struct page_pool *pool,
 	unsigned int max_size = PAGE_SIZE << pool->p.order;
 	struct page *page = pool->frag_page;
 
-	if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
-		    size > max_size))
+	if (WARN_ON(!pool->page_frag || size > max_size))
 		return NULL;
 
 	size = ALIGN(size, dma_get_cache_alignment());
@@ -774,7 +775,7 @@  static void page_pool_free(struct page_pool *pool)
 
 	ptr_ring_cleanup(&pool->ring, NULL);
 
-	if (pool->p.flags & PP_FLAG_DMA_MAP)
+	if (pool->dma_map)
 		put_device(pool->p.dev);
 
 #ifdef CONFIG_PAGE_POOL_STATS