On 10/06/2023 15.13, Yunsheng Lin wrote:
On 2023/6/9 23:02, Jesper Dangaard Brouer wrote:
...
PP_FLAG_DMA_SYNC_DEV |\
PP_FLAG_PAGE_FRAG)
+#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
+ (sizeof(dma_addr_t) > sizeof(unsigned long))
+
I have a problem with the name PAGE_POOL_DMA_USE_PP_FRAG_COUNT
because it is confusing to read in an if-statement.
Actually, it is already in an if-statement before this patch:)
I did notice, but I've had a problem with this name for a while.
(see later, why this might be long in separate patch)
Maybe starting to use it in the driver is confusing to you?
If not, maybe we can keep it that for now, and change it when
we come up with a better name.
Proposals rename to: DMA_OVERLAP_PP_FRAG_COUNT
Or: MM_DMA_OVERLAP_PP_FRAG_COUNT
Or: DMA_ADDR_OVERLAP_PP_FRAG_COUNT
It seems DMA_ADDR_OVERLAP_PP_FRAG_COUNT is better,
and DMA_ADDR_UPPER_OVERLAP_PP_FRAG_COUNT seems more accurate if a
longer macro name is not an issue here.
I like the shorter DMA_ADDR_OVERLAP_PP_FRAG_COUNT variant best.
Notice how I also removed the prefix PAGE_POOL_ because this is a
MM-layer constraint and not a property of page_pool.
I am not sure if it is a MM-layer constraint yet.
Do you mean 'MM-layer constraint' as 'struct page' not having
enough space for page pool with 32-bit arch with 64-bit DMA?
Yes.
If that is the case, we may need a more generic name for that
constraint instead of 'DMA_ADDR_OVERLAP_PP_FRAG_COUNT'?
I think this name is clear enough; the dma_addr_t is overlapping the
pp_frag_count.
And a more generic name seems confusing for page pool too, as
it doesn't tell that we only have that problem for 32-bit arch
with 64-bit DMA.
So if the above makes sense, it seems we may need to keep the
PAGE_POOL_ prefix, which would be
'PAGE_POOL_DMA_ADDR_UPPER_OVERLAP_PP_FRAG_COUNT' if the long
name is not issue here.
I think it gets too long now.
Also I still disagree with PAGE_POOL_ prefix, if anything it is a
property of 'struct page'. Thus a prefix with PAGE_ make more sense to
me, but it also gets too long (for my taste).
Anyway, naming is hard, we may need a seperate patch to explain
it, which is not really related to this patchset IHMO, so I'd
rather keep it as before if we can not come up with a name which
is not confusing to most people.
Okay, lets do the (re)naming in another patch then.
--Jesper
