Messages in this thread | | | From | Jesper Dangaard Brouer <> | Date | Sun, 11 Jun 2023 12:47:42 +0200 | Subject | Re: [PATCH net-next v3 1/4] page_pool: frag API support for 32-bit arch with 64-bit DMA |
| |
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
| |