netdev - Re: [RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr

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: <20190207152034.GA3295@apalos>
Date:   Thu, 7 Feb 2019 17:20:34 +0200
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     brouer@...hat.com, tariqt@...lanox.com, toke@...hat.com,
        davem@...emloft.net, netdev@...r.kernel.org,
        mgorman@...hsingularity.net, linux-mm@...ck.org
Subject: Re: [RFC, PATCH] net: page_pool: Don't use page->private to store
 dma_addr_t

Hi Matthew,

On Thu, Feb 07, 2019 at 07:07:45AM -0800, Matthew Wilcox wrote:
> On Thu, Feb 07, 2019 at 04:36:36PM +0200, Ilias Apalodimas wrote:
> > +/* Until we can update struct-page, have a shadow struct-page, that
> > + * include our use-case
> > + * Used to store retrieve dma addresses from network drivers.
> > + * Never access this directly, use helper functions provided
> > + * page_pool_get_dma_addr()
> > + */
> 
> Huh?  Why not simply:
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 2c471a2c43fa..2495a93ad90c 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -28,6 +28,10 @@ struct address_space;
>  struct mem_cgroup;
>  struct hmm;
>  
> +struct page_pool {
> +       dma_addr_t dma_addr;
> +};
> +
>  /*
>   * Each physical page in the system has a struct page associated with
>   * it to keep track of whatever it is we are using the page for at the
> @@ -77,6 +81,7 @@ struct page {
>          * avoid collision and false-positive PageTail().
>          */
>         union {
> +               struct page_pool pool;
>                 struct {        /* Page cache and anonymous pages */
>                         /**
>                          * @lru: Pageout list, eg. active_list protected by
> 

Well updating struct page is the final goal, hence the comment. I am mostly
looking for opinions here since we are trying to store dma addresses which are
irrelevant to pages. Having dma_addr_t definitions in mm-related headers is a
bit controversial isn't it ? If we can add that, then yes the code would look
better

Thanks
/Ilias