Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Christoph Hellwig]

On Wed, Oct 16, 2024 at 11:52:39AM -0700, David Wei wrote:
> From: Pavel Begunkov <asml.silence@gmail.com>
> 
> Currently net_iov stores a pointer to struct dmabuf_genpool_chunk_owner,
> which serves as a useful abstraction to share data and provide a
> context. However, it's too devmem specific, and we want to reuse it for
> other memory providers, and for that we need to decouple net_iov from
> devmem. Make net_iov to point to a new base structure called
> net_iov_area, which dmabuf_genpool_chunk_owner extends.

We've been there before.  Instead of reinventing your own memory
provider please enhance dmabufs for your use case.  We don't really
need to build memory buffer abstraction over memory buffer abstraction.

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Pavel Begunkov]

On 10/23/24 08:20, Christoph Hellwig wrote:
> On Wed, Oct 16, 2024 at 11:52:39AM -0700, David Wei wrote:
>> From: Pavel Begunkov <asml.silence@gmail.com>
>>
>> Currently net_iov stores a pointer to struct dmabuf_genpool_chunk_owner,
>> which serves as a useful abstraction to share data and provide a
>> context. However, it's too devmem specific, and we want to reuse it for
>> other memory providers, and for that we need to decouple net_iov from
>> devmem. Make net_iov to point to a new base structure called
>> net_iov_area, which dmabuf_genpool_chunk_owner extends.
> 
> We've been there before.  Instead of reinventing your own memory
> provider please enhance dmabufs for your use case.  We don't really
> need to build memory buffer abstraction over memory buffer abstraction.

It doesn't care much what kind of memory it is, nor it's important
for internals how it's imported, it's user addresses -> pages for
user convenience sake. All the net_iov setup code is in the page pool
core code. What it does, however, is implementing the user API, so
There is no relevance with dmabufs.

-- 
Pavel Begunkov

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Christoph Hellwig]

On Wed, Oct 23, 2024 at 03:34:53PM +0100, Pavel Begunkov wrote:
> It doesn't care much what kind of memory it is, nor it's important
> for internals how it's imported, it's user addresses -> pages for
> user convenience sake. All the net_iov setup code is in the page pool
> core code. What it does, however, is implementing the user API, so

That's not what this series does.  It adds the new memory_provider_ops
set of hooks, with once implementation for dmabufs, and one for
io_uring zero copy.

So you are precluding zero copy RX into anything but your magic
io_uring buffers, and using an odd abstraction for that.

The right way would be to support zero copy RX into every
designated dmabuf, and make io_uring work with udmabuf or if
absolutely needed it's own kind of dmabuf.  Instead we create
a maze of incompatible abstractions here.  The use case of e.g.
doing zero copy receive into a NVMe CMB using PCIe P2P transactions
is every but made up, so this does create a problem.

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Pavel Begunkov]

On 10/24/24 10:23, Christoph Hellwig wrote:
> On Wed, Oct 23, 2024 at 03:34:53PM +0100, Pavel Begunkov wrote:
>> It doesn't care much what kind of memory it is, nor it's important
>> for internals how it's imported, it's user addresses -> pages for
>> user convenience sake. All the net_iov setup code is in the page pool
>> core code. What it does, however, is implementing the user API, so
> 
> That's not what this series does.  It adds the new memory_provider_ops
> set of hooks, with once implementation for dmabufs, and one for
> io_uring zero copy.

First, it's not a _new_ abstraction over a buffer as you called it
before, the abstraction (net_iov) is already merged.

Second, you mention devmem TCP, and it's not just a page pool with
"dmabufs", it's a user API to use it and other memory agnostic
allocation logic. And yes, dmabufs there is the least technically
important part. Just having a dmabuf handle solves absolutely nothing.

> So you are precluding zero copy RX into anything but your magic
> io_uring buffers, and using an odd abstraction for that.

Right io_uring zero copy RX API expects transfer to happen into io_uring
controlled buffers, and that's the entire idea. Buffers that are based
on an existing network specific abstraction, which are not restricted to
pages or anything specific in the long run, but the flow of which from
net stack to user and back is controlled by io_uring. If you worry about
abuse, io_uring can't even sanely initialise those buffers itself and
therefore asking the page pool code to do that.

> The right way would be to support zero copy RX into every
> designated dmabuf, and make io_uring work with udmabuf or if

I have no idea what you mean, but shoving dmabufs into every single
place regardless whether it makes sense or not is hardly a good
way forward.

> absolutely needed it's own kind of dmabuf.  Instead we create

I'm even more confused how that would help. The user API has to
be implemented and adding a new dmabuf gives nothing, not even
mentioning it's not clear what semantics of that beast is
supposed to be.

> a maze of incompatible abstractions here.  The use case of e.g.
> doing zero copy receive into a NVMe CMB using PCIe P2P transactions
> is every but made up, so this does create a problem.

That's some kind of a confusion again, there is no reason why
it can't be supported, transparently to the non-setup code at
that. That's left out as other bits to further iterations to
keep this set simpler.

-- 
Pavel Begunkov

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Christoph Hellwig]

On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote:
> > That's not what this series does.  It adds the new memory_provider_ops
> > set of hooks, with once implementation for dmabufs, and one for
> > io_uring zero copy.
> 
> First, it's not a _new_ abstraction over a buffer as you called it
> before, the abstraction (net_iov) is already merged.

Umm, it is a new ops vector.

> Second, you mention devmem TCP, and it's not just a page pool with
> "dmabufs", it's a user API to use it and other memory agnostic
> allocation logic. And yes, dmabufs there is the least technically
> important part. Just having a dmabuf handle solves absolutely nothing.

It solves a lot, becaue it provides a proper abstraction.

> > So you are precluding zero copy RX into anything but your magic
> > io_uring buffers, and using an odd abstraction for that.
> 
> Right io_uring zero copy RX API expects transfer to happen into io_uring
> controlled buffers, and that's the entire idea. Buffers that are based
> on an existing network specific abstraction, which are not restricted to
> pages or anything specific in the long run, but the flow of which from
> net stack to user and back is controlled by io_uring. If you worry about
> abuse, io_uring can't even sanely initialise those buffers itself and
> therefore asking the page pool code to do that.

No, I worry about trying to io_uring for not good reason. This
pre-cludes in-kernel uses which would be extremly useful for
network storage drivers, and it precludes device memory of all
kinds.

> I'm even more confused how that would help. The user API has to
> be implemented and adding a new dmabuf gives nothing, not even
> mentioning it's not clear what semantics of that beast is
> supposed to be.
>

The dma-buf maintainers already explained to you last time
that there is absolutely no need to use the dmabuf UAPI, you
can use dma-bufs through in-kernel interfaces just fine.

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Pavel Begunkov]

On 10/24/24 17:06, Christoph Hellwig wrote:
> On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote:
>>> That's not what this series does.  It adds the new memory_provider_ops
>>> set of hooks, with once implementation for dmabufs, and one for
>>> io_uring zero copy.
>>
>> First, it's not a _new_ abstraction over a buffer as you called it
>> before, the abstraction (net_iov) is already merged.
> 
> Umm, it is a new ops vector.

I don't understand what you mean. Callback?

>> Second, you mention devmem TCP, and it's not just a page pool with
>> "dmabufs", it's a user API to use it and other memory agnostic
>> allocation logic. And yes, dmabufs there is the least technically
>> important part. Just having a dmabuf handle solves absolutely nothing.
> 
> It solves a lot, becaue it provides a proper abstraction.

Then please go ahead and take a look at the patchset in question
and see how much of dmabuf handling is there comparing to pure
networking changes. The point that it's a new set of API and lots
of changes not related directly to dmabufs stand. dmabufs is useful
there as an abstraction there, but it's a very long stretch saying
that the series is all about it.

> 
>>> So you are precluding zero copy RX into anything but your magic
>>> io_uring buffers, and using an odd abstraction for that.
>>
>> Right io_uring zero copy RX API expects transfer to happen into io_uring
>> controlled buffers, and that's the entire idea. Buffers that are based
>> on an existing network specific abstraction, which are not restricted to
>> pages or anything specific in the long run, but the flow of which from
>> net stack to user and back is controlled by io_uring. If you worry about
>> abuse, io_uring can't even sanely initialise those buffers itself and
>> therefore asking the page pool code to do that.
> 
> No, I worry about trying to io_uring for not good reason. This

It sounds that the argument is that you just don't want any
io_uring APIs, I don't think you'd be able to help you with
that.

> pre-cludes in-kernel uses which would be extremly useful for

Uses of what? devmem TCP is merged, I'm not removing it,
and the net_iov abstraction is in there, which can be potentially
be reused by other in-kernel users if that'd even make sense.

> network storage drivers, and it precludes device memory of all
> kinds.

You can't use page pools to allocate for a storage device, it's
a network specific allocator. You can get a dmabuf around that
device's memory and zero copy into it, but there is no problem
with that. Either use devmem TCP or wait until io_uring adds
support for dmabufs, which is, again, trivial.

>> I'm even more confused how that would help. The user API has to
>> be implemented and adding a new dmabuf gives nothing, not even
>> mentioning it's not clear what semantics of that beast is
>> supposed to be.
>>
> 
> The dma-buf maintainers already explained to you last time
> that there is absolutely no need to use the dmabuf UAPI, you
> can use dma-bufs through in-kernel interfaces just fine.

You can, even though it's not needed and I don't see how
it'd be useful, but you're missing the point. A new dmabuf
implementation doesn't implement the uapi we need nor it
helps to talk to the net layer.

-- 
Pavel Begunkov

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Christoph Hellwig]

On Thu, Oct 24, 2024 at 05:40:02PM +0100, Pavel Begunkov wrote:
> On 10/24/24 17:06, Christoph Hellwig wrote:
> > On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote:
> > > > That's not what this series does.  It adds the new memory_provider_ops
> > > > set of hooks, with once implementation for dmabufs, and one for
> > > > io_uring zero copy.
> > > 
> > > First, it's not a _new_ abstraction over a buffer as you called it
> > > before, the abstraction (net_iov) is already merged.
> > 
> > Umm, it is a new ops vector.
> 
> I don't understand what you mean. Callback?

struct memory_provider_ops.  It's a method table or ops vetor, no
callbacks involved.

> Then please go ahead and take a look at the patchset in question
> and see how much of dmabuf handling is there comparing to pure
> networking changes. The point that it's a new set of API and lots
> of changes not related directly to dmabufs stand. dmabufs is useful
> there as an abstraction there, but it's a very long stretch saying
> that the series is all about it.

I did take a look, that's why I replied.

> > > on an existing network specific abstraction, which are not restricted to
> > > pages or anything specific in the long run, but the flow of which from
> > > net stack to user and back is controlled by io_uring. If you worry about
> > > abuse, io_uring can't even sanely initialise those buffers itself and
> > > therefore asking the page pool code to do that.
> > 
> > No, I worry about trying to io_uring for not good reason. This
> 
> It sounds that the argument is that you just don't want any
> io_uring APIs, I don't think you'd be able to help you with
> that.

No, that's complete misinterpreting what I'm saying.  Of course an
io_uring API is fine.  But tying low-level implementation details to
to is not.

> > pre-cludes in-kernel uses which would be extremly useful for
> 
> Uses of what? devmem TCP is merged, I'm not removing it,
> and the net_iov abstraction is in there, which can be potentially
> be reused by other in-kernel users if that'd even make sense.

How when you are hardcoding io uring memory registrations instead
of making them a generic dmabuf?  Which btw would also really help
with pre-registering the memry with the iommu to get good performance
in IOMMU-enabled setups.

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Pavel Begunkov]

On 10/28/24 12:11, Christoph Hellwig wrote:
> On Thu, Oct 24, 2024 at 05:40:02PM +0100, Pavel Begunkov wrote:
>> On 10/24/24 17:06, Christoph Hellwig wrote:
>>> On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote:
>>>>> That's not what this series does.  It adds the new memory_provider_ops
>>>>> set of hooks, with once implementation for dmabufs, and one for
>>>>> io_uring zero copy.
>>>>
>>>> First, it's not a _new_ abstraction over a buffer as you called it
>>>> before, the abstraction (net_iov) is already merged.
>>>
>>> Umm, it is a new ops vector.
>>
>> I don't understand what you mean. Callback?
> 
> struct memory_provider_ops.  It's a method table or ops vetor, no
> callbacks involved.

I see, the reply is about your phrase about additional memory
abstractions:

"... don't really need to build memory buffer abstraction over
memory buffer abstraction."

>> Then please go ahead and take a look at the patchset in question
>> and see how much of dmabuf handling is there comparing to pure
>> networking changes. The point that it's a new set of API and lots
>> of changes not related directly to dmabufs stand. dmabufs is useful
>> there as an abstraction there, but it's a very long stretch saying
>> that the series is all about it.
> 
> I did take a look, that's why I replied.
> 
>>>> on an existing network specific abstraction, which are not restricted to
>>>> pages or anything specific in the long run, but the flow of which from
>>>> net stack to user and back is controlled by io_uring. If you worry about
>>>> abuse, io_uring can't even sanely initialise those buffers itself and
>>>> therefore asking the page pool code to do that.
>>>
>>> No, I worry about trying to io_uring for not good reason. This
>>
>> It sounds that the argument is that you just don't want any
>> io_uring APIs, I don't think you'd be able to help you with
>> that.
> 
> No, that's complete misinterpreting what I'm saying.  Of course an
> io_uring API is fine.  But tying low-level implementation details to
> to is not.

It works with low level concepts, i.e. private NIC queues, but it does
that through well established abstractions (page pool) already extended
for such cases. There is no directly going into a driver / hardware and
hard coding queue allocation, some memory injection or anything similar.
The user api has to embrace the hardware limitations, right, there is no
way around it without completely changing the approach and performance
and/or applicability. And queues as first class citizens is not a new
concept in general.

>>> pre-cludes in-kernel uses which would be extremly useful for
>>
>> Uses of what? devmem TCP is merged, I'm not removing it,
>> and the net_iov abstraction is in there, which can be potentially
>> be reused by other in-kernel users if that'd even make sense.
> 
> How when you are hardcoding io uring memory registrations instead
> of making them a generic dmabuf?  Which btw would also really help

If you mean internals, making up a dmabuf that has never existed in the
picture in the first place is not cleaner or easier in any way. If that
changes, e.g. there is more code to reuse in the future, we can unify it
then.

If that's about user api, you've just mentioned before that it can be
pages / user pointers. As to why it goes through io_uring, I explained
it before, but in short, it gives a better api for io_uring users, we
can avoid creating a yet another file (netlink socket) and keeping it
around, that way we don't need to synchronise with the nl socket and/or
trying to steal memory from it, and the devmem api is also too
monolithic for such purposes, so even that would need to change, i.e.
splitting queue and memory registration.

> with pre-registering the memry with the iommu to get good performance
> in IOMMU-enabled setups.

The page pool already does that just like it handles the normal
path without providers.

-- 
Pavel Begunkov

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

[Christoph Hellwig]

On Tue, Oct 29, 2024 at 04:35:16PM +0000, Pavel Begunkov wrote:
> I see, the reply is about your phrase about additional memory
> abstractions:
> 
> "... don't really need to build memory buffer abstraction over
> memory buffer abstraction."

Yes, over the exsting memory buffer abstraction (dma_buf).

> If you mean internals, making up a dmabuf that has never existed in the
> picture in the first place is not cleaner or easier in any way. If that
> changes, e.g. there is more code to reuse in the future, we can unify it
> then.

I'm not sure what "making up" means here, they are all made up :)

> > with pre-registering the memry with the iommu to get good performance
> > in IOMMU-enabled setups.
> 
> The page pool already does that just like it handles the normal
> path without providers.

In which case is basically is a dma-buf.  If you'd expose it as such
we could actually use to communicate between subsystems in the
kernel.