Re: [RFC PATCH net-next v5 07/14] page_pool: devmem support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/13/24 21:11, Mina Almasry wrote:
On Tue, Feb 13, 2024 at 5:28 AM Pavel Begunkov <asml.silence@xxxxxxxxx> 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




[Index of Archives]     [Linux SoC]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux