On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
> + /* Return error here to avoid mlx5e_page_release_fragmented()
> + * calling page_pool_defrag_page() to write to pp_frag_count
> + * which is overlapped with dma_addr_upper in 'struct page' for
> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> + */
> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> + err = -EINVAL;
> + goto err_free_by_rq_type;
> + }
I told you not to do this in a comment on v4.
Keep the flag in page pool params and let the creation fail.
On 2023/7/8 7:59, Jakub Kicinski wrote:
> On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
>> + /* Return error here to avoid mlx5e_page_release_fragmented()
>> + * calling page_pool_defrag_page() to write to pp_frag_count
>> + * which is overlapped with dma_addr_upper in 'struct page' for
>> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
>> + */
>> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
>> + err = -EINVAL;
>> + goto err_free_by_rq_type;
>> + }
>
> I told you not to do this in a comment on v4.
> Keep the flag in page pool params and let the creation fail.
There seems to be naming disagreement in the previous discussion,
The simplest way seems to be reuse the
PAGE_POOL_DMA_USE_PP_FRAG_COUNT and do the checking in the driver
without introducing new macro or changing macro name.
Let's be more specific about what is your suggestion here:
Do you mean keep the PP_FLAG_PAGE_FRAG flag and keep the below
checking in page_pool_init(), right?
if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
pool->p.flags & PP_FLAG_PAGE_FRAG)
return -EINVAL;
Isn't it confusing to still say page frag is not supported
for PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true case when this
patch will now add support for it, at least from API POV, the
page_pool_alloc_frag() is always supported now.
Maybe remove the PP_FLAG_PAGE_FRAG and add a new macro named
PP_FLAG_PAGE_SPLIT_IN_DRIVER, and do the checking as before in
page_pool_init() like below?
if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
return -EINVAL;
Or any better suggestion?
On Sun, 9 Jul 2023 20:39:45 +0800 Yunsheng Lin wrote:
> On 2023/7/8 7:59, Jakub Kicinski wrote:
> > On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
> >> + /* Return error here to avoid mlx5e_page_release_fragmented()
> >> + * calling page_pool_defrag_page() to write to pp_frag_count
> >> + * which is overlapped with dma_addr_upper in 'struct page' for
> >> + * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> >> + */
> >> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> >> + err = -EINVAL;
> >> + goto err_free_by_rq_type;
> >> + }
> >
> > I told you not to do this in a comment on v4.
> > Keep the flag in page pool params and let the creation fail.
>
> There seems to be naming disagreement in the previous discussion,
> The simplest way seems to be reuse the
> PAGE_POOL_DMA_USE_PP_FRAG_COUNT and do the checking in the driver
> without introducing new macro or changing macro name.
>
> Let's be more specific about what is your suggestion here:
> Do you mean keep the PP_FLAG_PAGE_FRAG flag and keep the below
> checking in page_pool_init(), right?
> if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> pool->p.flags & PP_FLAG_PAGE_FRAG)
> return -EINVAL;
>
> Isn't it confusing to still say page frag is not supported
> for PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true case when this
> patch will now add support for it, at least from API POV, the
> page_pool_alloc_frag() is always supported now.
I don't mind what the flag is called, I just want the check to stay
inside the page_pool code, acting on driver info passed inside
pp_params.
> Maybe remove the PP_FLAG_PAGE_FRAG and add a new macro named
> PP_FLAG_PAGE_SPLIT_IN_DRIVER, and do the checking as before in
> page_pool_init() like below?
> if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
> return -EINVAL;
Yup, that sound good.