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

[Matthew Iselin <matthew@xxxxxxxxxxxxxx>]

On Wed, Jan 27, 2010 at 7:54 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
> 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.
>
> <snip>
>
> 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.

I must have misunderstood your initial explanation of
cdi_mem_require_flags. In light of the information in this newest
mail...

So cdi_mem_require_flags would take a buffer that might be passed from
an external source, and ensure that the buffer matches the given
flags? If so that's a good idea and my only nitpick would be to ask
for a rename ;)

>> 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.

Right. I think this is one spot where the semantics of the functions
need to be unambiguously defined (very clearly ;) )... Documentation
here should be very in-depth in order to avoid misunderstandings like
the one I experienced.

> 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.

I'd say leave it up to the driver to ask for a page-aligned area. In
fact, a better idea would be to have alignments on different word
boundaries, as some devices are more than happy to accept buffers
which are not page aligned but rather aligned on an n-word or n-byte
boundary.

Cheers,
Matt