Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] page_pool: mark unbound node page as reusable pages
@ 2019-12-04 10:14 Li RongQing
  2019-12-05  0:55 ` Yunsheng Lin
  0 siblings, 1 reply; 14+ messages in thread
From: Li RongQing @ 2019-12-04 10:14 UTC (permalink / raw)
  To: netdev, saeedm

some drivers uses page pool, but not require to allocate
page from bound node, so pool.p.nid is NUMA_NO_NODE, and
this fixed patch will block this kind of driver to
recycle

Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
---
 net/core/page_pool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a6aefe989043..4054db683178 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct page *page,
  */
 static bool pool_page_reusable(struct page_pool *pool, struct page *page)
 {
-	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
+	return !page_is_pfmemalloc(page) &&
+		(page_to_nid(page) == pool->p.nid ||
+		 pool->p.nid == NUMA_NO_NODE);
 }
 
 void __page_pool_put_page(struct page_pool *pool, struct page *page,
-- 
2.16.2


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-04 10:14 [PATCH] page_pool: mark unbound node page as reusable pages Li RongQing
@ 2019-12-05  0:55 ` Yunsheng Lin
  2019-12-05  1:08   ` 答复: " Li,Rongqing
  2019-12-05  1:22   ` Li,Rongqing
  0 siblings, 2 replies; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  0:55 UTC (permalink / raw)
  To: Li RongQing, netdev, saeedm

On 2019/12/4 18:14, Li RongQing wrote:
> some drivers uses page pool, but not require to allocate
> page from bound node, so pool.p.nid is NUMA_NO_NODE, and
> this fixed patch will block this kind of driver to
> recycle
> 
> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> ---
>  net/core/page_pool.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a6aefe989043..4054db683178 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct page *page,
>   */
>  static bool pool_page_reusable(struct page_pool *pool, struct page *page)
>  {
> -	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
> +	return !page_is_pfmemalloc(page) &&
> +		(page_to_nid(page) == pool->p.nid ||
> +		 pool->p.nid == NUMA_NO_NODE);

If I understand it correctly, you are allowing recycling when
pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
log: "this fixed patch will block this kind of driver to recycle".

Maybe you mean "commit d5394610b1ba" by this fixed patch?

Also, maybe it is better to allow recycling if the below condition
is matched:

	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) == numa_mem_id()

>  }
>  
>  void __page_pool_put_page(struct page_pool *pool, struct page *page,
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  0:55 ` Yunsheng Lin
@ 2019-12-05  1:08   ` " Li,Rongqing
  2019-12-05  1:43     ` Yunsheng Lin
  2019-12-05  1:22   ` Li,Rongqing
  1 sibling, 1 reply; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  1:08 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm



> -----邮件原件-----
> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> 发送时间: 2019年12月5日 8:55
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> saeedm@mellanox.com
> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable pages
> 
> On 2019/12/4 18:14, Li RongQing wrote:
> > some drivers uses page pool, but not require to allocate page from
> > bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch will
> > block this kind of driver to recycle
> >
> > Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  net/core/page_pool.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
> > a6aefe989043..4054db683178 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct page
> *page,
> >   */
> >  static bool pool_page_reusable(struct page_pool *pool, struct page
> > *page)  {
> > -	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
> > +	return !page_is_pfmemalloc(page) &&
> > +		(page_to_nid(page) == pool->p.nid ||
> > +		 pool->p.nid == NUMA_NO_NODE);
> 
> If I understand it correctly, you are allowing recycling when
> pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
> log: "this fixed patch will block this kind of driver to recycle".
> 
> Maybe you mean "commit d5394610b1ba" by this fixed patch?

yes

> 
> Also, maybe it is better to allow recycling if the below condition is matched:
> 
> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
> numa_mem_id()

If driver uses NUMA_NO_NODE, it does not care numa node, and maybe its platform
Only has a node, so not need to compare like "page_to_nid(page) ==  numa_mem_id()"


-RongQing


> 
> >  }
> >
> >  void __page_pool_put_page(struct page_pool *pool, struct page *page,
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  0:55 ` Yunsheng Lin
  2019-12-05  1:08   ` 答复: " Li,Rongqing
@ 2019-12-05  1:22   ` Li,Rongqing
  1 sibling, 0 replies; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  1:22 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm



> -----邮件原件-----
> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> 发送时间: 2019年12月5日 8:55
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> saeedm@mellanox.com
> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable pages
> 
> On 2019/12/4 18:14, Li RongQing wrote:
> > some drivers uses page pool, but not require to allocate page from
> > bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch will
> > block this kind of driver to recycle
> >
> > Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  net/core/page_pool.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
> > a6aefe989043..4054db683178 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct page
> *page,
> >   */
> >  static bool pool_page_reusable(struct page_pool *pool, struct page
> > *page)  {
> > -	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
> > +	return !page_is_pfmemalloc(page) &&
> > +		(page_to_nid(page) == pool->p.nid ||
> > +		 pool->p.nid == NUMA_NO_NODE);
> 
> If I understand it correctly, you are allowing recycling when
> pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
> log: "this fixed patch will block this kind of driver to recycle".
> 
> Maybe you mean "commit d5394610b1ba" by this fixed patch?
> 


I maybe should change the commit header as below:


    page_pool: take unbound numa node allocated pages as reusable
    
    some drivers uses page pool, but not require to allocate
    page from bound node, so pool.p.nid is NUMA_NO_NODE, and
    the commit d5394610b1ba ("page_pool: Don't recycle
    non-reusable pages") will block this kind of driver to
    recycle
    
    Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")

Thanks

-RongQing


> Also, maybe it is better to allow recycling if the below condition is matched:
> 
> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
> numa_mem_id()
> 
> >  }
> >
> >  void __page_pool_put_page(struct page_pool *pool, struct page *page,
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  1:08   ` 答复: " Li,Rongqing
@ 2019-12-05  1:43     ` Yunsheng Lin
  2019-12-05  1:55       ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  1:43 UTC (permalink / raw)
  To: Li,Rongqing, netdev, saeedm

On 2019/12/5 9:08, Li,Rongqing wrote:
> 
> 
>> -----邮件原件-----
>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>> 发送时间: 2019年12月5日 8:55
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>> saeedm@mellanox.com
>> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable pages
>>
>> On 2019/12/4 18:14, Li RongQing wrote:
>>> some drivers uses page pool, but not require to allocate page from
>>> bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch will
>>> block this kind of driver to recycle
>>>
>>> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>>> ---
>>>  net/core/page_pool.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
>>> a6aefe989043..4054db683178 100644
>>> --- a/net/core/page_pool.c
>>> +++ b/net/core/page_pool.c
>>> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct page
>> *page,
>>>   */
>>>  static bool pool_page_reusable(struct page_pool *pool, struct page
>>> *page)  {
>>> -	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
>>> +	return !page_is_pfmemalloc(page) &&
>>> +		(page_to_nid(page) == pool->p.nid ||
>>> +		 pool->p.nid == NUMA_NO_NODE);
>>
>> If I understand it correctly, you are allowing recycling when
>> pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
>> log: "this fixed patch will block this kind of driver to recycle".
>>
>> Maybe you mean "commit d5394610b1ba" by this fixed patch?
> 
> yes
> 
>>
>> Also, maybe it is better to allow recycling if the below condition is matched:
>>
>> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
>> numa_mem_id()
> 
> If driver uses NUMA_NO_NODE, it does not care numa node, and maybe its platform
> Only has a node, so not need to compare like "page_to_nid(page) ==  numa_mem_id()"

Normally, driver does not care if the node of a device is NUMA_NO_NODE or not, it
just uses the node that returns from dev_to_node().

Even for multi node system, the node of a device may be NUMA_NO_NODE when
BIOS/FW has not specified it through ACPI/DT, see [1].


[1] https://lore.kernel.org/patchwork/patch/1141952/

> 
> 
> -RongQing
> 
> 
>>
>>>  }
>>>
>>>  void __page_pool_put_page(struct page_pool *pool, struct page *page,
>>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  1:43     ` Yunsheng Lin
@ 2019-12-05  1:55       ` " Li,Rongqing
  2019-12-05  2:06         ` Yunsheng Lin
  0 siblings, 1 reply; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  1:55 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm



> -----邮件原件-----
> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> 发送时间: 2019年12月5日 9:44
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> saeedm@mellanox.com
> 主题: Re: 答复: [PATCH] page_pool: mark unbound node page as reusable
> pages
> 
> On 2019/12/5 9:08, Li,Rongqing wrote:
> >
> >
> >> -----邮件原件-----
> >> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> >> 发送时间: 2019年12月5日 8:55
> >> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> >> saeedm@mellanox.com
> >> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable pages
> >>
> >> On 2019/12/4 18:14, Li RongQing wrote:
> >>> some drivers uses page pool, but not require to allocate page from
> >>> bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch will
> >>> block this kind of driver to recycle
> >>>
> >>> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> >>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> >>> Cc: Saeed Mahameed <saeedm@mellanox.com>
> >>> ---
> >>>  net/core/page_pool.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
> >>> a6aefe989043..4054db683178 100644
> >>> --- a/net/core/page_pool.c
> >>> +++ b/net/core/page_pool.c
> >>> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct
> >>> page
> >> *page,
> >>>   */
> >>>  static bool pool_page_reusable(struct page_pool *pool, struct page
> >>> *page)  {
> >>> -	return !page_is_pfmemalloc(page) && page_to_nid(page) ==
> pool->p.nid;
> >>> +	return !page_is_pfmemalloc(page) &&
> >>> +		(page_to_nid(page) == pool->p.nid ||
> >>> +		 pool->p.nid == NUMA_NO_NODE);
> >>
> >> If I understand it correctly, you are allowing recycling when
> >> pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
> >> log: "this fixed patch will block this kind of driver to recycle".
> >>
> >> Maybe you mean "commit d5394610b1ba" by this fixed patch?
> >
> > yes
> >
> >>
> >> Also, maybe it is better to allow recycling if the below condition is matched:
> >>
> >> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
> >> numa_mem_id()
> >
> > If driver uses NUMA_NO_NODE, it does not care numa node, and maybe its
> > platform Only has a node, so not need to compare like "page_to_nid(page) ==
> numa_mem_id()"
> 
> Normally, driver does not care if the node of a device is NUMA_NO_NODE or
> not, it just uses the node that returns from dev_to_node().
> 
> Even for multi node system, the node of a device may be NUMA_NO_NODE
> when BIOS/FW has not specified it through ACPI/DT, see [1].
> 
> 
> [1] https://lore.kernel.org/patchwork/patch/1141952/
> 

at this condition, page can be allocated from any node from driver boot,
why need to check "page_to_nid(page) == numa_mem_id()" at recycle?

-Li 

> >
> >
> > -RongQing
> >
> >
> >>
> >>>  }
> >>>
> >>>  void __page_pool_put_page(struct page_pool *pool, struct page
> >>> *page,
> >>>
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  1:55       ` 答复: " Li,Rongqing
@ 2019-12-05  2:06         ` Yunsheng Lin
  2019-12-05  2:17           ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  2:06 UTC (permalink / raw)
  To: Li,Rongqing, netdev, saeedm

On 2019/12/5 9:55, Li,Rongqing wrote:
> 
> 
>> -----邮件原件-----
>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>> 发送时间: 2019年12月5日 9:44
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>> saeedm@mellanox.com
>> 主题: Re: 答复: [PATCH] page_pool: mark unbound node page as reusable
>> pages
>>
>> On 2019/12/5 9:08, Li,Rongqing wrote:
>>>
>>>
>>>> -----邮件原件-----
>>>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>>>> 发送时间: 2019年12月5日 8:55
>>>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>>>> saeedm@mellanox.com
>>>> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable pages
>>>>
>>>> On 2019/12/4 18:14, Li RongQing wrote:
>>>>> some drivers uses page pool, but not require to allocate page from
>>>>> bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch will
>>>>> block this kind of driver to recycle
>>>>>
>>>>> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
>>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>>>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>>>>> ---
>>>>>  net/core/page_pool.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
>>>>> a6aefe989043..4054db683178 100644
>>>>> --- a/net/core/page_pool.c
>>>>> +++ b/net/core/page_pool.c
>>>>> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct
>>>>> page
>>>> *page,
>>>>>   */
>>>>>  static bool pool_page_reusable(struct page_pool *pool, struct page
>>>>> *page)  {
>>>>> -	return !page_is_pfmemalloc(page) && page_to_nid(page) ==
>> pool->p.nid;
>>>>> +	return !page_is_pfmemalloc(page) &&
>>>>> +		(page_to_nid(page) == pool->p.nid ||
>>>>> +		 pool->p.nid == NUMA_NO_NODE);
>>>>
>>>> If I understand it correctly, you are allowing recycling when
>>>> pool->p.nid is NUMA_NO_NODE, which does not seems match the commit
>>>> log: "this fixed patch will block this kind of driver to recycle".
>>>>
>>>> Maybe you mean "commit d5394610b1ba" by this fixed patch?
>>>
>>> yes
>>>
>>>>
>>>> Also, maybe it is better to allow recycling if the below condition is matched:
>>>>
>>>> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
>>>> numa_mem_id()
>>>
>>> If driver uses NUMA_NO_NODE, it does not care numa node, and maybe its
>>> platform Only has a node, so not need to compare like "page_to_nid(page) ==
>> numa_mem_id()"
>>
>> Normally, driver does not care if the node of a device is NUMA_NO_NODE or
>> not, it just uses the node that returns from dev_to_node().
>>
>> Even for multi node system, the node of a device may be NUMA_NO_NODE
>> when BIOS/FW has not specified it through ACPI/DT, see [1].
>>
>>
>> [1] https://lore.kernel.org/patchwork/patch/1141952/
>>
> 
> at this condition, page can be allocated from any node from driver boot,
> why need to check "page_to_nid(page) == numa_mem_id()" at recycle?

For performance, the performance is better when the rx page is on the same
node as the rx process is running.

We want the node of rx page is close to the node of device/cpu to achive
better performance, since the node of device is unknown, maybe we choose
the node of memory that is close to the cpu that is running to handle the
rx cleaning.

> 
> -Li 
> 
>>>
>>>
>>> -RongQing
>>>
>>>
>>>>
>>>>>  }
>>>>>
>>>>>  void __page_pool_put_page(struct page_pool *pool, struct page
>>>>> *page,
>>>>>
>>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  2:06         ` Yunsheng Lin
@ 2019-12-05  2:17           ` " Li,Rongqing
  2019-12-05  2:30             ` Yunsheng Lin
  0 siblings, 1 reply; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  2:17 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm



> -----邮件原件-----
> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> 发送时间: 2019年12月5日 10:06
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> saeedm@mellanox.com
> 主题: Re: 答复: 答复: [PATCH] page_pool: mark unbound node page as
> reusable pages
> 
> On 2019/12/5 9:55, Li,Rongqing wrote:
> >
> >
> >> -----邮件原件-----
> >> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> >> 发送时间: 2019年12月5日 9:44
> >> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> >> saeedm@mellanox.com
> >> 主题: Re: 答复: [PATCH] page_pool: mark unbound node page as reusable
> >> pages
> >>
> >> On 2019/12/5 9:08, Li,Rongqing wrote:
> >>>
> >>>
> >>>> -----邮件原件-----
> >>>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
> >>>> 发送时间: 2019年12月5日 8:55
> >>>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
> >>>> saeedm@mellanox.com
> >>>> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable
> pages
> >>>>
> >>>> On 2019/12/4 18:14, Li RongQing wrote:
> >>>>> some drivers uses page pool, but not require to allocate page from
> >>>>> bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch
> >>>>> will block this kind of driver to recycle
> >>>>>
> >>>>> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable
> >>>>> pages")
> >>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> >>>>> Cc: Saeed Mahameed <saeedm@mellanox.com>
> >>>>> ---
> >>>>>  net/core/page_pool.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
> >>>>> a6aefe989043..4054db683178 100644
> >>>>> --- a/net/core/page_pool.c
> >>>>> +++ b/net/core/page_pool.c
> >>>>> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct
> >>>>> page
> >>>> *page,
> >>>>>   */
> >>>>>  static bool pool_page_reusable(struct page_pool *pool, struct
> >>>>> page
> >>>>> *page)  {
> >>>>> -	return !page_is_pfmemalloc(page) && page_to_nid(page) ==
> >> pool->p.nid;
> >>>>> +	return !page_is_pfmemalloc(page) &&
> >>>>> +		(page_to_nid(page) == pool->p.nid ||
> >>>>> +		 pool->p.nid == NUMA_NO_NODE);
> >>>>
> >>>> If I understand it correctly, you are allowing recycling when
> >>>> pool->p.nid is NUMA_NO_NODE, which does not seems match the
> commit
> >>>> log: "this fixed patch will block this kind of driver to recycle".
> >>>>
> >>>> Maybe you mean "commit d5394610b1ba" by this fixed patch?
> >>>
> >>> yes
> >>>
> >>>>
> >>>> Also, maybe it is better to allow recycling if the below condition is
> matched:
> >>>>
> >>>> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
> >>>> numa_mem_id()
> >>>
> >>> If driver uses NUMA_NO_NODE, it does not care numa node, and maybe
> >>> its platform Only has a node, so not need to compare like
> >>> "page_to_nid(page) ==
> >> numa_mem_id()"
> >>
> >> Normally, driver does not care if the node of a device is
> >> NUMA_NO_NODE or not, it just uses the node that returns from
> dev_to_node().
> >>
> >> Even for multi node system, the node of a device may be NUMA_NO_NODE
> >> when BIOS/FW has not specified it through ACPI/DT, see [1].
> >>
> >>
> >> [1] https://lore.kernel.org/patchwork/patch/1141952/
> >>
> >
> > at this condition, page can be allocated from any node from driver
> > boot, why need to check "page_to_nid(page) == numa_mem_id()" at recycle?
> 
> For performance, the performance is better when the rx page is on the same
> node as the rx process is running.
> 
> We want the node of rx page is close to the node of device/cpu to achive better
> performance, since the node of device is unknown, maybe we choose the node
> of memory that is close to the cpu that is running to handle the rx cleaning.
> 

if the driver takes care about numa node, it should not assign NUMA_NO_NODE, it should
assign a detail numa node at starting step. Not depend on recycle to decide the numa
node

-RongQing


> >
> > -Li
> >
> >>>
> >>>
> >>> -RongQing
> >>>
> >>>
> >>>>
> >>>>>  }
> >>>>>
> >>>>>  void __page_pool_put_page(struct page_pool *pool, struct page
> >>>>> *page,
> >>>>>
> >>>
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  2:17           ` 答复: " Li,Rongqing
@ 2019-12-05  2:30             ` Yunsheng Lin
  2019-12-05  2:47               ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  2:30 UTC (permalink / raw)
  To: Li,Rongqing, netdev, saeedm

>> -----邮件原件-----
>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>> 发送时间: 2019年12月5日 10:06
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>> saeedm@mellanox.com
>> 主题: Re: 答复: 答复: [PATCH] page_pool: mark unbound node page as
>> reusable pages
>>
>> On 2019/12/5 9:55, Li,Rongqing wrote:
>>>
>>>
>>>> -----邮件原件-----
>>>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>>>> 发送时间: 2019年12月5日 9:44
>>>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>>>> saeedm@mellanox.com
>>>> 主题: Re: 答复: [PATCH] page_pool: mark unbound node page as reusable
>>>> pages
>>>>
>>>> On 2019/12/5 9:08, Li,Rongqing wrote:
>>>>>
>>>>>
>>>>>> -----邮件原件-----
>>>>>> 发件人: Yunsheng Lin [mailto:linyunsheng@huawei.com]
>>>>>> 发送时间: 2019年12月5日 8:55
>>>>>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org;
>>>>>> saeedm@mellanox.com
>>>>>> 主题: Re: [PATCH] page_pool: mark unbound node page as reusable
>> pages
>>>>>>
>>>>>> On 2019/12/4 18:14, Li RongQing wrote:
>>>>>>> some drivers uses page pool, but not require to allocate page from
>>>>>>> bound node, so pool.p.nid is NUMA_NO_NODE, and this fixed patch
>>>>>>> will block this kind of driver to recycle
>>>>>>>
>>>>>>> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable
>>>>>>> pages")
>>>>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>>>>>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>>>>>>> ---
>>>>>>>  net/core/page_pool.c | 4 +++-
>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c index
>>>>>>> a6aefe989043..4054db683178 100644
>>>>>>> --- a/net/core/page_pool.c
>>>>>>> +++ b/net/core/page_pool.c
>>>>>>> @@ -317,7 +317,9 @@ static bool __page_pool_recycle_direct(struct
>>>>>>> page
>>>>>> *page,
>>>>>>>   */
>>>>>>>  static bool pool_page_reusable(struct page_pool *pool, struct
>>>>>>> page
>>>>>>> *page)  {
>>>>>>> -	return !page_is_pfmemalloc(page) && page_to_nid(page) ==
>>>> pool->p.nid;
>>>>>>> +	return !page_is_pfmemalloc(page) &&
>>>>>>> +		(page_to_nid(page) == pool->p.nid ||
>>>>>>> +		 pool->p.nid == NUMA_NO_NODE);
>>>>>>
>>>>>> If I understand it correctly, you are allowing recycling when
>>>>>> pool->p.nid is NUMA_NO_NODE, which does not seems match the
>> commit
>>>>>> log: "this fixed patch will block this kind of driver to recycle".
>>>>>>
>>>>>> Maybe you mean "commit d5394610b1ba" by this fixed patch?
>>>>>
>>>>> yes
>>>>>
>>>>>>
>>>>>> Also, maybe it is better to allow recycling if the below condition is
>> matched:
>>>>>>
>>>>>> 	pool->p.nid == NUMA_NO_NODE && page_to_nid(page) ==
>>>>>> numa_mem_id()
>>>>>
>>>>> If driver uses NUMA_NO_NODE, it does not care numa node, and maybe
>>>>> its platform Only has a node, so not need to compare like
>>>>> "page_to_nid(page) ==
>>>> numa_mem_id()"
>>>>
>>>> Normally, driver does not care if the node of a device is
>>>> NUMA_NO_NODE or not, it just uses the node that returns from
>> dev_to_node().
>>>>
>>>> Even for multi node system, the node of a device may be NUMA_NO_NODE
>>>> when BIOS/FW has not specified it through ACPI/DT, see [1].
>>>>
>>>>
>>>> [1] https://lore.kernel.org/patchwork/patch/1141952/
>>>>
>>>
>>> at this condition, page can be allocated from any node from driver
>>> boot, why need to check "page_to_nid(page) == numa_mem_id()" at recycle?
>>
>> For performance, the performance is better when the rx page is on the same
>> node as the rx process is running.
>>
>> We want the node of rx page is close to the node of device/cpu to achive better
>> performance, since the node of device is unknown, maybe we choose the node
>> of memory that is close to the cpu that is running to handle the rx cleaning.
>>
> 
> if the driver takes care about numa node, it should not assign NUMA_NO_NODE, it should
> assign a detail numa node at starting step. Not depend on recycle to decide the numa
> node

How and where we should handle the NUMA_NO_NODE has been discussed before,
see [1].
but the driver has not been considered the place to handle it.

For driver that has not using page pool, the numa_mem_id() checking
is how they decide to recycle or not, see [2] [3].

I think it is better to be consistent with the page pool too.


[1] https://lore.kernel.org/patchwork/patch/1125789/
[2] https://elixir.bootlin.com/linux/v5.4.2/source/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c#L2437
[3] https://elixir.bootlin.com/linux/v5.4.2/source/drivers/net/ethernet/intel/i40e/i40e_txrx.c#L1858
> 
> -RongQing
> 
> 
>>>
>>> -Li
>>>
>>>>>
>>>>>
>>>>> -RongQing
>>>>>
>>>>>
>>>>>>
>>>>>>>  }
>>>>>>>
>>>>>>>  void __page_pool_put_page(struct page_pool *pool, struct page
>>>>>>> *page,
>>>>>>>
>>>>>
>>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  2:30             ` Yunsheng Lin
@ 2019-12-05  2:47               ` " Li,Rongqing
  2019-12-05  3:03                 ` Yunsheng Lin
  0 siblings, 1 reply; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  2:47 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm

> 
> [1] https://lore.kernel.org/patchwork/patch/1125789/


What is status of this patch? I think you should fix your firmware or bios

Consider the below condition:

there is two numa node, and NIC sites on node 2, but NUMA_NO_NODE is used, recycle will fail due to page_to_nid(page) == numa_mem_id(), 
and reallocated pages maybe always from node 1, then the recycle will never success.

-RongQing



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 答复: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  2:47               ` 答复: " Li,Rongqing
@ 2019-12-05  3:03                 ` Yunsheng Lin
  2019-12-05  3:18                   ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  3:03 UTC (permalink / raw)
  To: Li,Rongqing, netdev, saeedm

On 2019/12/5 10:47, Li,Rongqing wrote:
>>
>> [1] https://lore.kernel.org/patchwork/patch/1125789/
> 
> 
> What is status of this patch? I think you should fix your firmware or bios

Have not reached a conclusion yet.

> 
> Consider the below condition:
> 
> there is two numa node, and NIC sites on node 2, but NUMA_NO_NODE is used, recycle will fail due to page_to_nid(page) == numa_mem_id(), 
> and reallocated pages maybe always from node 1, then the recycle will never success.

For page pool:

1. if the pool->p.nid != NUMA_NO_NODE, the recycle is always decided by
checking page_to_nid(page) == pool->p.nid.

2. only when the pool->p.nid == NUMA_NO_NODE, the numa_mem_id() is checked
   to decide the recycle.

Yes, If pool->p.nid == NUMA_NO_NODE, and the cpu that is doing recycling
is changing each time, the recycle may never success, but it is not common,
and have its own performance penalty when changing the recycling cpu so
often.


> 
> -RongQing
> 
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: 答复: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  3:03                 ` Yunsheng Lin
@ 2019-12-05  3:18                   ` " Li,Rongqing
  2019-12-05  3:33                     ` Yunsheng Lin
  0 siblings, 1 reply; 14+ messages in thread
From: Li,Rongqing @ 2019-12-05  3:18 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm


> >> [1] https://lore.kernel.org/patchwork/patch/1125789/
> >
> >
> > What is status of this patch? I think you should fix your firmware or
> > bios
> 
> Have not reached a conclusion yet.

I think it will never be accepted

> 
> >
> > Consider the below condition:
> >
> > there is two numa node, and NIC sites on node 2, but NUMA_NO_NODE is
> > used, recycle will fail due to page_to_nid(page) == numa_mem_id(), and
> reallocated pages maybe always from node 1, then the recycle will never
> success.
> 
> For page pool:
> 
> 1. if the pool->p.nid != NUMA_NO_NODE, the recycle is always decided by
> checking page_to_nid(page) == pool->p.nid.
> 
> 2. only when the pool->p.nid == NUMA_NO_NODE, the numa_mem_id() is
> checked
>    to decide the recycle.
> 
> Yes, If pool->p.nid == NUMA_NO_NODE, and the cpu that is doing recycling is
> changing each time, the recycle may never success, but it is not common, 

Why can we ignore this condition, and accept your hardware with abnormal node
information

>and
> have its own performance penalty when changing the recycling cpu so often.

I have said if hardware takes care of numa node, it should be assign when page pool
Is created, not depends on recycle.

If you insist your idea, you can submit you patch after this one

-RongQing

> 
> 
> >
> > -RongQing
> >
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: 答复: 答复: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  3:18                   ` 答复: " Li,Rongqing
@ 2019-12-05  3:33                     ` Yunsheng Lin
  2019-12-06  8:05                       ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 14+ messages in thread
From: Yunsheng Lin @ 2019-12-05  3:33 UTC (permalink / raw)
  To: Li,Rongqing, netdev, saeedm

On 2019/12/5 11:18, Li,Rongqing wrote:
> 
>>>> [1] https://lore.kernel.org/patchwork/patch/1125789/
>>>
>>>
>>> What is status of this patch? I think you should fix your firmware or
>>> bios
>>
>> Have not reached a conclusion yet.
> 
> I think it will never be accepted
> 
>>
>>>
>>> Consider the below condition:
>>>
>>> there is two numa node, and NIC sites on node 2, but NUMA_NO_NODE is
>>> used, recycle will fail due to page_to_nid(page) == numa_mem_id(), and
>> reallocated pages maybe always from node 1, then the recycle will never
>> success.
>>
>> For page pool:
>>
>> 1. if the pool->p.nid != NUMA_NO_NODE, the recycle is always decided by
>> checking page_to_nid(page) == pool->p.nid.
>>
>> 2. only when the pool->p.nid == NUMA_NO_NODE, the numa_mem_id() is
>> checked
>>    to decide the recycle.
>>
>> Yes, If pool->p.nid == NUMA_NO_NODE, and the cpu that is doing recycling is
>> changing each time, the recycle may never success, but it is not common, 
> 
> Why can we ignore this condition, and accept your hardware with abnormal node
> information
> 
>> and
>> have its own performance penalty when changing the recycling cpu so often.
> 
> I have said if hardware takes care of numa node, it should be assign when page pool
> Is created, not depends on recycle.
> 
> If you insist your idea, you can submit you patch after this one

I am just arguing that rx recycle the page pool is doing should be consistent
with other driver that does its own recycle.

If the driver that does its own recycle begin to support the page pool,
then there may be different behavior when they are not consistent.

> 
> -RongQing
> 
>>
>>
>>>
>>> -RongQing
>>>
>>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* 答复: 答复: 答复: 答复: 答复: 答复: [PATCH] page_pool: mark unbound node page as reusable pages
  2019-12-05  3:33                     ` Yunsheng Lin
@ 2019-12-06  8:05                       ` " Li,Rongqing
  0 siblings, 0 replies; 14+ messages in thread
From: Li,Rongqing @ 2019-12-06  8:05 UTC (permalink / raw)
  To: Yunsheng Lin, netdev, saeedm

> I am just arguing that rx recycle the page pool is doing should be consistent
> with other driver that does its own recycle.
> 
> If the driver that does its own recycle begin to support the page pool, then
> there may be different behavior when they are not consistent.
> 

OK, I will send v2, with your suggestion

Thanks

-Li

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 10:14 [PATCH] page_pool: mark unbound node page as reusable pages Li RongQing
2019-12-05  0:55 ` Yunsheng Lin
2019-12-05  1:08   ` 答复: " Li,Rongqing
2019-12-05  1:43     ` Yunsheng Lin
2019-12-05  1:55       ` 答复: " Li,Rongqing
2019-12-05  2:06         ` Yunsheng Lin
2019-12-05  2:17           ` 答复: " Li,Rongqing
2019-12-05  2:30             ` Yunsheng Lin
2019-12-05  2:47               ` 答复: " Li,Rongqing
2019-12-05  3:03                 ` Yunsheng Lin
2019-12-05  3:18                   ` 答复: " Li,Rongqing
2019-12-05  3:33                     ` Yunsheng Lin
2019-12-06  8:05                       ` 答复: " Li,Rongqing
2019-12-05  1:22   ` Li,Rongqing

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git