netdev - Re: [PATCH RFC 04/10] mm: page_frag: introduce page_frag_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKgT0UfouCZpX04yzvCrB_UBmy47p+=xm5qViYowerR9dPcCbg@mail.gmail.com>
Date: Mon, 28 Oct 2024 10:53:33 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Linux-MM <linux-mm@...ck.org>, 
	Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org
Subject: Re: [PATCH RFC 04/10] mm: page_frag: introduce page_frag_alloc_abort()
 related API

On Mon, Oct 28, 2024 at 5:05 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> For some case as tun_build_skb() without the needing of
> using complicated prepare & commit API, add the abort API to
> abort the operation of page_frag_alloc_*() related API for
> error handling knowing that no one else is taking extra
> reference to the just allocated fragment, and add abort_ref
> API to only abort the reference counting of the allocated
> fragment if it is already referenced by someone else.
>
> CC: Alexander Duyck <alexander.duyck@...il.com>
> CC: Andrew Morton <akpm@...ux-foundation.org>
> CC: Linux-MM <linux-mm@...ck.org>
> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
> ---
>  Documentation/mm/page_frags.rst |  7 +++++--
>  include/linux/page_frag_cache.h | 20 ++++++++++++++++++++
>  mm/page_frag_cache.c            | 21 +++++++++++++++++++++
>  3 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/mm/page_frags.rst b/Documentation/mm/page_frags.rst
> index 34e654c2956e..339e641beb53 100644
> --- a/Documentation/mm/page_frags.rst
> +++ b/Documentation/mm/page_frags.rst
> @@ -114,9 +114,10 @@ fragsz if there is an alignment requirement for the size of the fragment.
>  .. kernel-doc:: include/linux/page_frag_cache.h
>     :identifiers: page_frag_cache_init page_frag_cache_is_pfmemalloc
>                  __page_frag_alloc_align page_frag_alloc_align page_frag_alloc
> +                page_frag_alloc_abort
>
>  .. kernel-doc:: mm/page_frag_cache.c
> -   :identifiers: page_frag_cache_drain page_frag_free
> +   :identifiers: page_frag_cache_drain page_frag_free page_frag_alloc_abort_ref
>
>  Coding examples
>  ===============
> @@ -143,8 +144,10 @@ Allocation & freeing API
>          goto do_error;
>
>      err = do_something(va, size);
> -    if (err)
> +    if (err) {
> +        page_frag_alloc_abort(nc, va, size);
>          goto do_error;
> +    }
>
>      ...
>
> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
> index a2b1127e8ac8..c3347c97522c 100644
> --- a/include/linux/page_frag_cache.h
> +++ b/include/linux/page_frag_cache.h
> @@ -141,5 +141,25 @@ static inline void *page_frag_alloc(struct page_frag_cache *nc,
>  }
>
>  void page_frag_free(void *addr);
> +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va,
> +                              unsigned int fragsz);
> +
> +/**
> + * page_frag_alloc_abort - Abort the page fragment allocation.
> + * @nc: page_frag cache to which the page fragment is aborted back
> + * @va: virtual address of page fragment to be aborted
> + * @fragsz: size of the page fragment to be aborted
> + *
> + * It is expected to be called from the same context as the allocation API.
> + * Mostly used for error handling cases to abort the fragment allocation knowing
> + * that no one else is taking extra reference to the just aborted fragment, so
> + * that the aborted fragment can be reused.
> + */
> +static inline void page_frag_alloc_abort(struct page_frag_cache *nc, void *va,
> +                                        unsigned int fragsz)
> +{
> +       page_frag_alloc_abort_ref(nc, va, fragsz);
> +       nc->offset -= fragsz;
> +}
>
>  #endif
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index d014130fb893..4d5626da42ed 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -201,3 +201,24 @@ void page_frag_free(void *addr)
>                 free_unref_page(page, compound_order(page));
>  }
>  EXPORT_SYMBOL(page_frag_free);
> +
> +/**
> + * page_frag_alloc_abort_ref - Abort the reference of allocated fragment.
> + * @nc: page_frag cache to which the page fragment is aborted back
> + * @va: virtual address of page fragment to be aborted
> + * @fragsz: size of the page fragment to be aborted
> + *
> + * It is expected to be called from the same context as the allocation API.
> + * Mostly used for error handling cases to abort the reference of allocated
> + * fragment if the fragment has been referenced for other usages, to aovid the
> + * atomic operation of page_frag_free() API.
> + */
> +void page_frag_alloc_abort_ref(struct page_frag_cache *nc, void *va,
> +                              unsigned int fragsz)
> +{
> +       VM_BUG_ON(va + fragsz !=
> +                 encoded_page_decode_virt(nc->encoded_page) + nc->offset);
> +
> +       nc->pagecnt_bias++;
> +}
> +EXPORT_SYMBOL(page_frag_alloc_abort_ref);

It isn't clear to me why you split this over two functions. It seems
like you could just update the offset in this lower function rather
than do it in the upper one since you are passing all the arguments
here anyway.