| Message ID | 1575454465-15386-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | page_pool: mark unbound node page as reusable pages | expand |
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, >
> -----邮件原件----- > 发件人: 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, > >
> -----邮件原件----- > 发件人: 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, > >
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, >>> >
> -----邮件原件----- > 发件人: 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, > >>> > >
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, >>>>> >>> >
> -----邮件原件----- > 发件人: 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, > >>>>> > >>> > >
>> -----邮件原件----- >> 发件人: 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, >>>>>>> >>>>> >>> >
> > [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
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 > >
> >> [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 > > > >
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 >>> >>> >
> 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
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,
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(-)