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

[Matthew Iselin <matthew@xxxxxxxxxxxxxx>]

On Thu, Jan 28, 2010 at 6:58 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
> On Wed, Jan 27, 2010 at 09:51:24PM +1000, Matthew Iselin wrote:
>> On Wed, Jan 27, 2010 at 7:54 AM, Kevin Wolf <kevin@xxxxxxxxxx> wrote:
>> > 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 ;)
>
> Yes, I think this is what I meant. I'm open for suggestions for a better
> name.

cdi_adjust_buffer perhaps? Something that actually says "this function
may adjust the buffer's characteristics to that of the flags in the
parameter list"...

>> >> 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.
>
> Unfortunately, it seems when I try to explain things very clearly it's
> almost guaranteed that you are going to misunderstand them. ;-)

I try ;)

> Maybe you could modify the patch so it fits your idea of a
> well-documented clean header?

If you want I can do that tonight.

>> > 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.
>
> That's a very good point, actually. Should we reserve some bits in the
> flags for it, make it another parameter or even a new function for
> allocating/requiring aligned memory?

Perhaps a new function in which you can specify the number of bytes to
align against, such as cdi_buffer_align(void *, int).

> Another point that came to my mind today is that maybe we shouldn't have
> a single paddr in the memory area, but maintain a scatter/gather list of
> physical addresses.

You mean the CDI implementation provides a set of buffers which CDI
drivers ask for and use? What would be its purpose?

Cheers,
Matt