Re: [cdi-devel] [RFC PATCH] mem.h: Handling memory areas

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Wed, Jan 27, 2010 at 06:54:06AM +1000, Matthew Iselin wrote:
> On Mon, Jan 25, 2010 at 7:37 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
> > <snip>
> >
> > To explain the use of the other functions, consider the following.
> > Let's take a disk driver as an example. Instead of buffer/size its
> > write function will directly get a memory area from the OS. The OS might
> > already have allocated the memory physically contiguous, but it doesn't
> > need to do so. The driver calls cdi_mem_require_flags which copies the
> > data into a suited buffer - but if the OS already did the right thing,
> > it's a no-op.
> 
> Hmm, I don't actually like the sounds of that. I'd prefer that when
> the driver gets memory from the OS it specifies the flags it requires
> for the memory area rather than get the memory then change flags on
> the area (potentially requiring a full reallocation of the buffer
> itself) later.

I'm not sure if we're talking about the same thing here. There are
basically two different scenarios to consider and you could mean any of
them:

1) The driver needs some memory internally. This one is easy, it simply
calls cdi_mem_alloc with the right flags and gets what it requested.

2) The driver doesn't explicitly allocate memory but gets it from the OS
as a parameter to a function like the buffer for send_packet. This is
more tricky. The data usually comes from a userspace program in this
case, so the memory can only have the flag it happens to fulfill when the
memory is passed to the kernel (assuming a monolithic model).

At this point, we have two possibilities: The kernel can just pass the
unchanged memory area down and the driver needs to make it fit its
requrements (this is what I was trying to achieve with
cdi_mem_require_flags). The only alternative that I see is to specify
the required flags for each memory area parameter of any callback
function in the driver struct. I personally consider this really awkward.

But maybe you have another idea how this could be solved.

> I also don't think cdi_mem_require_flags should perform
> a copy: semantics like that are not intuitive. I may have
> mis-interpreted of course.

Not sure what it should do instead?

> > cdi_mem_copy is for the other way round. Now the disk driver wants to
> > read something from its disk. It gets a memory area from the OS and uses
> > cdi_mem_require_flags to get something usable for DMA, which may or may
> > not involve a copy. When the disk has transferred all the data, the
> > driver uses cdi_mem_copy to make sure that the read data ends up in the
> > buffer it was passed - and again, if the buffer had already the right
> > flags this will be a no-op.
> 
> Again, cdi_mem_require_flags probably shouldn't perform a copy.
> 
> It just sounds like the interface could be cleaned up a bit, and
> especially could be modified to reduce the amount of copying that the
> CDI implementation might have to do. Drivers at least should know what
> they need from the very beginning.

The whole thing was made to avoid copies in the first place. But there
are cases where you just need to copy data. So what I tried is to
introduce functions which transparently do the copy or are a no-op when
no copy is needed.

Obviously I failed with either designing or explaining them. ;-)

> Apart from these nitpicks I like where you're going with the interface.

This is not nitpicks. It's important questions on the interface and I'm
happy that you're asking them. I feel much more comfortable with
committing some code after a discussion because I know that people have
really thought about it. I'd like to hear your suggestions for the
memory interface.

By the way, one more thing that I wasn't sure about is if it's really
helpful to restrict the whole thing to a page granularity. Maybe this
should just be a flag CDI_MEM_PAGE_ALIGN which could be used by drivers
whenever it's helpful to use complete pages. Or maybe leave it
completely to the OS.

Kevin