On Tue, 22 Oct 2019 04:44:21 +0000
Saeed Mahameed <sae...@mellanox.com> wrote:
> A page is NOT reusable when at least one of the following is true:
> 1) allocated when system was under some pressure. (page_is_pfmemalloc)
> 2) belongs to a different NUMA node than pool->p.nid.
>
> To update pool->p.nid users should call page_pool_update_nid().
>
> Holding on to such pages in the pool will hurt the consumer performance
> when the pool migrates to a different numa node.
>
> Performance testing:
> XDP drop/tx rate and TCP single/multi stream, on mlx5 driver
> while migrating rx ring irq from close to far numa:
>
> mlx5 internal page cache was locally disabled to get pure page pool
> results.
>
> CPU: Intel(R) Xeon(R) CPU E5-2603 v4 @ 1.70GHz
> NIC: Mellanox Technologies MT27700 Family [ConnectX-4] (100G)
>
> XDP Drop/TX single core:
> NUMA | XDP | Before | After
> ---------------------------------------
> Close | Drop | 11 Mpps | 10.8 Mpps
> Far | Drop | 4.4 Mpps | 5.8 Mpps
>
> Close | TX | 6.5 Mpps | 6.5 Mpps
> Far | TX | 4 Mpps | 3.5 Mpps
>
> Improvement is about 30% drop packet rate, 15% tx packet rate for numa
> far test.
> No degradation for numa close tests.
>
> TCP single/multi cpu/stream:
> NUMA | #cpu | Before | After
> --------------------------------------
> Close | 1 | 18 Gbps | 18 Gbps
> Far | 1 | 15 Gbps | 18 Gbps
> Close | 12 | 80 Gbps | 80 Gbps
> Far | 12 | 68 Gbps | 80 Gbps
>
> In all test cases we see improvement for the far numa case, and no
> impact on the close numa case.
>
> The impact of adding a check per page is very negligible, and shows no
> performance degradation whatsoever, also functionality wise it seems more
> correct and more robust for page pool to verify when pages should be
> recycled, since page pool can't guarantee where pages are coming from.
>
> Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
> Acked-by: Jonathan Lemon <jonathan.le...@gmail.com>
> ---
> net/core/page_pool.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 08ca9915c618..8120aec999ce 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -283,6 +283,17 @@ static bool __page_pool_recycle_direct(struct page *page,
> return true;
> }
>
> +/* page is NOT reusable when:
> + * 1) allocated when system is under some pressure. (page_is_pfmemalloc)
> + * 2) belongs to a different NUMA node than pool->p.nid.
> + *
> + * To update pool->p.nid users must call page_pool_update_nid.
> + */
> +static bool pool_page_reusable(struct page_pool *pool, struct page *page)
> +{
> + return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
I think we have discussed this before. You are adding the
page_is_pfmemalloc(page) memory pressure test, even-though the
allocation side of page_pool will not give us these kind of pages.
I'm going to accept this anyway, as it is a good safeguard, as it is a
very bad thing to recycle such a page. Performance wise, you have
showed it have almost zero impact, which I guess is because we are
already reading the struct page area here.
> +}
> +
> void __page_pool_put_page(struct page_pool *pool,
> struct page *page, bool allow_direct)
> {
> @@ -292,7 +303,8 @@ void __page_pool_put_page(struct page_pool *pool,
> *
> * refcnt == 1 means page_pool owns page, and can recycle it.
> */
> - if (likely(page_ref_count(page) == 1)) {
> + if (likely(page_ref_count(page) == 1 &&
> + pool_page_reusable(pool, page))) {
> /* Read barrier done in page_ref_count / READ_ONCE */
>
> if (allow_direct && in_serving_softirq())
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
