Date: Wed, 14 Feb 2024 15:30:25 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-alpha@...r.kernel.org, linux-mips@...r.kernel.org,
 linux-parisc@...r.kernel.org, sparclinux@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
 bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jonathan Corbet <corbet@....net>,
 Richard Henderson <richard.henderson@...aro.org>,
 Ivan Kokshaysky <ink@...assic.park.msu.ru>, Matt Turner
 <mattst88@...il.com>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
 Helge Deller <deller@....de>, Jesper Dangaard Brouer <hawk@...nel.org>,
 Ilias Apalodimas <ilias.apalodimas@...aro.org>,
 Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>, David Ahern <dsahern@...nel.org>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Shuah Khan <shuah@...nel.org>, Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 David Wei <dw@...idwei.uk>, Jason Gunthorpe <jgg@...pe.ca>,
 Yunsheng Lin <linyunsheng@...wei.com>, Shailend Chand <shailend@...gle.com>,
 Harshitha Ramamurthy <hramamurthy@...gle.com>,
 Shakeel Butt <shakeelb@...gle.com>, Jeroen de Borst <jeroendb@...gle.com>,
 Praveen Kaligineedi <pkaligineedi@...gle.com>
Subject: Re: [RFC PATCH net-next v5 07/14] page_pool: devmem support

On 2/13/24 21:11, Mina Almasry wrote:
> On Tue, Feb 13, 2024 at 5:28 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>>
..
>>
>> A bit of a churn with the padding and nesting net_iov but looks
>> sturdier. No duplication, and you can just check positions of the
>> structure instead of per-field NET_IOV_ASSERT_OFFSET, which you
>> have to not forget to update e.g. when adding a new field. Also,
> 
> Yes, this is nicer. If possible I'll punt it to a minor cleanup as a
> follow up change. Logistically I think if this series need-not touch
> code outside of net/, that's better.

Outside of net it should only be a small change in struct page
layout, but otherwise with struct_group_tagged things like
page->pp_magic would still work. Anyway, I'm not insisting.


>> with the change __netmem_clear_lsb can return a pointer to that
>> structure, casting struct net_iov when it's a page is a bit iffy.
>>
>> And the next question would be whether it'd be a good idea to encode
>> iov vs page not by setting a bit but via one of the fields in the
>> structure, maybe pp_magic.
>>
> 
> I will push back against this, for 2 reasons:
> 
> 1. I think pp_magic's first 2 bits (and maybe more) are used by mm
> code and thus I think extending usage of pp_magic in this series is a
> bit iffy and I would like to avoid it. I just don't want to touch the
> semantics of struct page if I don't have to.
> 2. I think this will be a measurable perf regression. Currently we can
> tell if a pointer is a page or net_iov without dereferencing the
> pointer and dirtying the cache-line. This will cause us to possibly
> dereference the pointer in areas where we don't need to. I think I had
> an earlier version of this code that required a dereference to tell if
> a page was devmem and Eric pointed to me it was a perf regression.

fair enough

> I also don't see any upside of using pp_magic, other than making the
> code slightly more readable, maybe.
> 
>> With that said I'm a bit concerned about the net_iov size. If each
>> represents 4096 bytes and you're registering 10MB, then you need
>> 30 pages worth of memory just for the iov array. Makes kvmalloc
>> a must even for relatively small sizes.
>>
> 
> This I think is an age-old challenge with pages. 1.6% of the machine's
> memory is 'wasted' on every machine because a struct page needs to be
> allocated for each PAGE_SIZE region. We're running into the same issue
> here where if we want to refer to PAGE_SIZE regions of memory we need
> to allocate some reference to it. Note that net_iov can be relatively
> easily extended to support N order pages. Also note that in the devmem
> TCP use case it's not really an issue; the minor increase in mem
> utilization is more than offset by the saving in memory bw as compared
> to using host memory as a bounce buffer.

It's not about memory consumption per se but rather the need
to vmalloc everything because of size.

> All in all I vote this is
> something that can be tuned or improved in the future if someone finds
> the extra memory usage a hurdle to using devmem TCP or this net_iov
> infra.

That's exactly what I was saying about overlaying it with
struct page, where the increase in size came from, but I agree
it's not critical

>> And the final bit, I don't believe the overlay is necessary in
>> this series. Optimisations are great, but this one is a bit more on
>> the controversial side. Unless I missed something and it does make
>> things easier, it might make sense to do it separately later.
>>
> 
> I completely agree, the overlay is not necessary. I implemented the
> overlay in response to Yunsheng's  strong requests for more 'unified'
> processing between page and devmem. This is the most unification I can
> do IMO without violating the requirements from Jason. I'm prepared to
> remove the overlay if it turns out controversial, but so far I haven't
> seen any complaints. Jason, please do take a look if you have not
> already.

Just to be clear, I have no objections to the change but noting
that IMHO it can be removed for now if it'd be dragging down
the set.

-- 
Pavel Begunkov

Powered by blists - more mailing lists