Received: by 2002:a9a:4c47:0:b029:116:c383:538 with SMTP id u7csp862711lko; Tue, 13 Jul 2021 11:42:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwuGCab2YIU7oBjWUDu81rIWjVOgk890UFjT2TcH8WqidthUldrqAdI5o0Dr2B8KQkWr3R2 X-Received: by 2002:a05:6402:1d94:: with SMTP id dk20mr7490248edb.171.1626201778277; Tue, 13 Jul 2021 11:42:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1626201778; cv=none; d=google.com; s=arc-20160816; b=p6ofKXw8HZFRkPwaiH6GyEB4yCk9GyUblhprx6V2SPV04tavIw58o1kwKT64ppmFhe PZp0DewvQxU6Phm7lkdRdGjyGn8gV0lt9O59haRwW6YB+GhAxiGdivGxOXnDmpKY82Br VvBY2uQSnFeop7d8X5P76AYEyl/0/g7FCjzAykHEbzNBBwn+Z7DCX4Mmw++zJpinXoGl mNyI8t52aAdrY/Jp3/6iSYAZXZxaVAWSIRbGmxV9Fnda1fQbqQ6s1TbDBzsyfTPHcavO zy7R7xK/JVsgq67lElBOhmjfHuLDelD/AhY2p7YkaFJitn2nu2Zpb4r8cvQ8Aqk3nIfE NzhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=FnotVOBY4MJ516v1c1mv4RJWivhMUK0+SH3ZuHVwU20=; b=LWq4F3meXLTZTwWHU1FGljc/YC9qGsOJq2QQSccaMkoorV/7gorXRKeBMfY/LlwovI KTv5+2Pkf2LQ/fVK/+Qof74vnIKHkep1H1x3s3PhtayFYjXqrcoKSy+1i+eJVHRcHwRy Pc5OS7rMfpoA08TM5CnnB0lXJp3OEWJYNbErtiBKessINzVI+7zW8uTETywQy2PGwZj2 f82gicmSQs+Ni0BuNREKmgTL84a9Z9WYhR58vGzkFnWd4cxLCgzxQo/TrbM1R0dlejnG QTo5P9ZhdB8XlBuurrPoc6s4As7YWkooowfbXJX4zAxCniDwb/RZJX6Kd+hfOyytHx8l AUvQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b="ocH/cyHk"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id em21si20992902ejc.542.2021.07.13.11.42.36; Tue, 13 Jul 2021 11:42:58 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b="ocH/cyHk"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229478AbhGMSoK (ORCPT + 99 others); Tue, 13 Jul 2021 14:44:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46020 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234548AbhGMSoF (ORCPT ); Tue, 13 Jul 2021 14:44:05 -0400 Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 55A33C0613EE; Tue, 13 Jul 2021 11:41:14 -0700 (PDT) Received: by mail-ej1-x631.google.com with SMTP id c17so43203536ejk.13; Tue, 13 Jul 2021 11:41:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FnotVOBY4MJ516v1c1mv4RJWivhMUK0+SH3ZuHVwU20=; b=ocH/cyHkQ2xDE+OiGRIQa0nRf0KV/hVRl4c0T/OyflQIoFehVp0nhgd+IN3Ov7uE4m fqtDMQtuOgqtjdie2j9qlHulMyKHKrgo1dg1gtsbXr927nK14Nt0F7/k/2KtFRl41ZcC IRJTldIaWXNLmTzf798r+N9WDISyZ5kGDaP7mOgPBIwg16Co3XSO2MHVDg/DCIkAIBTg TNvVJJOYgGUhoxjdvbKyf+SDn35L7IF9JboMY1GE9HE4O2Gw9NapQt0a6tj9Wu08dA+A k54f6HrjM6m7mpXmpfQkVQiR7gswYRrkYOFCTXaGa+jIJaUSuDhV4rcURhdKtBztxLbb hK+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FnotVOBY4MJ516v1c1mv4RJWivhMUK0+SH3ZuHVwU20=; b=kFhejzADZCdRa3O1vaVc4Qtc3jqjA6mhBXXLIDZ6iKR7N/6qln6jV09w8Ow7kyxcvR 5CrhiTIOsno2aEnRdTlt/Q//4Yow0F5a7RTIg7yQDnZg437GwzaFDQEUbTf7StWIczR3 vRQJ3F33ZrZvioLodzu1i33BFshyhFm6b/PHTSPipkKrQRRlD0lhJYIyHyjMlvl/kxmr msU1O7mLpjGttcN2g2da9sCzUjrVUkG8cAJ+14OnqXaIH5SaPoTXtVJRJn5rAEKilgWM BQ3sWRZq/H499Gnp8msOuMEfCbBhDeM+NnEBx13JauEAeYIW6w53n1V2scv598GEa0kL kl8A== X-Gm-Message-State: AOAM533T6wZTIwEu9ujLC+3OBrb3yKxa7LB17L3+BqoD31yI200HziYp nVrzXjIbZE7a8N7MKBdU2UN2CKUhJxv1CJaAXRk= X-Received: by 2002:a17:907:e9e:: with SMTP id ho30mr7609884ejc.114.1626201672721; Tue, 13 Jul 2021 11:41:12 -0700 (PDT) MIME-Version: 1.0 References: <1626168272-25622-1-git-send-email-linyunsheng@huawei.com> <1626168272-25622-3-git-send-email-linyunsheng@huawei.com> In-Reply-To: <1626168272-25622-3-git-send-email-linyunsheng@huawei.com> From: Alexander Duyck Date: Tue, 13 Jul 2021 11:41:01 -0700 Message-ID: Subject: Re: [PATCH rfc v4 2/4] page_pool: add interface to manipulate bias in page pool To: Yunsheng Lin Cc: David Miller , Jakub Kicinski , Russell King - ARM Linux , Marcin Wojtas , linuxarm@openeuler.org, yisen.zhuang@huawei.com, Salil Mehta , thomas.petazzoni@bootlin.com, hawk@kernel.org, Ilias Apalodimas , Alexei Starovoitov , Daniel Borkmann , John Fastabend , Andrew Morton , Peter Zijlstra , Will Deacon , Matthew Wilcox , Vlastimil Babka , fenghua.yu@intel.com, guro@fb.com, Peter Xu , Feng Tang , Jason Gunthorpe , Matteo Croce , Hugh Dickins , Jonathan Lemon , Alexander Lobakin , Willem de Bruijn , wenxu@ucloud.cn, Cong Wang , Kevin Hao , nogikh@google.com, Marco Elver , Yonghong Song , kpsingh@kernel.org, andrii@kernel.org, Martin KaFai Lau , songliubraving@fb.com, Netdev , LKML , bpf Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 13, 2021 at 2:25 AM Yunsheng Lin wrote: > > As suggested by Alexander, "A DMA mapping should be page > aligned anyway so the lower 12 bits would be reserved 0", > so it might make more sense to repurpose the lower 12 bits > of the dma address to store the bias for frag page support > in page pool for 32 bit systems with 64 bit dma, which > should be rare those days. > > For normal system, the dma_addr[1] in 'struct page' is not > used, so we can reuse the dma_addr[1] for storing bias. > > The PAGE_POOP_USE_DMA_ADDR_1 macro is used to decide where > to store the bias, as the "sizeof(dma_addr_t) > sizeof( > unsigned long)" is false for normal system, so hopefully the > compiler will optimize out the unused code for those system. I assume the name is a typo and you meant PAGE_POOL_USE_DMA_ADDR_1? > The newly added page_pool_set_bias() should be called before > the page is passed to any user. Otherwise, call the newly > added page_pool_atomic_sub_bias_return(). > > Signed-off-by: Yunsheng Lin > --- > include/net/page_pool.h | 70 ++++++++++++++++++++++++++++++++++++++++++++++--- > net/core/page_pool.c | 10 +++++++ > 2 files changed, 77 insertions(+), 3 deletions(-) > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 8d7744d..315b9f2 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -198,21 +198,85 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, > page_pool_put_full_page(pool, page, true); > } > > +#define PAGE_POOP_USE_DMA_ADDR_1 (sizeof(dma_addr_t) > sizeof(unsigned long)) > + > static inline dma_addr_t page_pool_get_dma_addr(struct page *page) > { > - dma_addr_t ret = page->dma_addr[0]; > - if (sizeof(dma_addr_t) > sizeof(unsigned long)) > + dma_addr_t ret; > + > + if (PAGE_POOP_USE_DMA_ADDR_1) { > + ret = READ_ONCE(page->dma_addr[0]) & PAGE_MASK; > ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16; Alternatively we could change things a bit and rename things so we have the MSB of dma_addr where dma_addr[1] is and we rename dma_addr[0] to pp_frag_count we could have it also contain the lower bits and handle it like so: ret = page->dma_addr; if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) { ret <<= 32; ret |= atomic_long_read(&page->pp_frag_count) & PAGE_MASK; } > + } else { > + ret = page->dma_addr[0]; > + } > + > return ret; > } > > static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr) > { > page->dma_addr[0] = addr; > - if (sizeof(dma_addr_t) > sizeof(unsigned long)) > + if (PAGE_POOP_USE_DMA_ADDR_1) > page->dma_addr[1] = upper_32_bits(addr); So assuming similar logic to above we could do something like: if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) { atomic_long_set(&page->pp_frag_count, addr & PAGE_MASK); addr >>= 32; } pp->dma_addr = addr; > } > > +static inline int page_pool_atomic_sub_bias_return(struct page *page, int nr) > +{ > + int bias; > + > + if (PAGE_POOP_USE_DMA_ADDR_1) { > + unsigned long *bias_ptr = &page->dma_addr[0]; > + unsigned long old_bias = READ_ONCE(*bias_ptr); > + unsigned long new_bias; > + > + do { > + bias = (int)(old_bias & ~PAGE_MASK); > + > + /* Warn when page_pool_dev_alloc_pages() is called > + * with PP_FLAG_PAGE_FRAG flag in driver. > + */ > + WARN_ON(!bias); > + > + /* already the last user */ > + if (!(bias - nr)) > + return 0; > + > + new_bias = old_bias - nr; > + } while (!try_cmpxchg(bias_ptr, &old_bias, new_bias)); > + > + WARN_ON((new_bias & PAGE_MASK) != (old_bias & PAGE_MASK)); > + > + bias = new_bias & ~PAGE_MASK; > + } else { > + atomic_t *v = (atomic_t *)&page->dma_addr[1]; The problem with casting like this is that it makes assumptions about byte ordering in the case that atomic_t is a 32b value and dma_addr is a long value. > + > + if (atomic_read(v) == nr) > + return 0; > + > + bias = atomic_sub_return(nr, v); > + WARN_ON(bias < 0); > + } Rather than have 2 versions of this function it might work better to just use the atomic_long version of these functions instead. Then you shouldn't need to have two versions of the code. You could just modify the block on the end to check for new_frag_count vs old_frag_count if PAGE_POOL_USE_PP_FRAG_COUNT is true, or new_frag_count < 0 if false. > + > + return bias; > +} > + > +static inline void page_pool_set_bias(struct page *page, int bias) > +{ > + if (PAGE_POOP_USE_DMA_ADDR_1) { > + unsigned long dma_addr_0 = READ_ONCE(page->dma_addr[0]); > + > + dma_addr_0 &= PAGE_MASK; > + dma_addr_0 |= bias; > + > + WRITE_ONCE(page->dma_addr[0], dma_addr_0); > + } else { > + atomic_t *v = (atomic_t *)&page->dma_addr[1]; > + > + atomic_set(v, bias); > + } Similarly here you could just update bias to include the dma_addr in the if case, and then use atomic_long_set for both cases. > +} > + > static inline bool is_page_pool_compiled_in(void) > { > #ifdef CONFIG_PAGE_POOL > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 78838c6..6ac5b00 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -198,6 +198,16 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page) > if (dma_mapping_error(pool->p.dev, dma)) > return false; > > + if (PAGE_POOP_USE_DMA_ADDR_1 && > + WARN_ON(pool->p.flags & PP_FLAG_PAGE_FRAG && > + dma & ~PAGE_MASK)) { > + dma_unmap_page_attrs(pool->p.dev, dma, > + PAGE_SIZE << pool->p.order, > + pool->p.dma_dir, > + DMA_ATTR_SKIP_CPU_SYNC); > + return false; > + } > + > page_pool_set_dma_addr(page, dma); > > if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV) > -- > 2.7.4 >