ODP Buffer Management support for Scatter-gather list

From: Bala Manoharan <bala.manoharan@linaro.org>

This patch contains the initial draft of Buffer Management with support for scatter-gather list 

Regards,
Bala

Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
---
 include/odp_buffer.h |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

Hi All,

This patch is meant for discussion on scatter-gather buffer support.
The implementation of these functions will be submitted once the proposed
APIs are finalised.

Regards,
Bala


On 16 June 2014 16:11, <bala.manoharan@linaro.org> wrote:

> From: Bala Manoharan <bala.manoharan@linaro.org>
>
> This patch contains the initial draft of Buffer Management with support
> for scatter-gather list
>
> Regards,
> Bala
>
> Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
> ---
>  include/odp_buffer.h |   73
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
> index d79e76d..c313168 100644
> --- a/include/odp_buffer.h
> +++ b/include/odp_buffer.h
> @@ -92,6 +92,79 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>   */
>  void odp_buffer_print(odp_buffer_t buf);
>
> +/**
> +*Number of segments in this scatter-gather List
> +*
> +*@param buf    Buffer Handle
> +*
> +*@return       Total number of segments in this scatter-gather List.
> +*
> +**/
> +int odp_buffer_segment_count(odp_buffer_t buf)
> +
> +/**
> +*Address of the segment in the buffer list.
> +*
> +*@param buf    Buffer Handle
> +*@param n      Index number for the segment in the SG list
> +*@param size   Returns the size of the current addressable segment
> +
> +*@return       Segment start address
> +**/
> +
> +void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
> +
> +/*
> +*Creates ODP Buffer from a memory region
> +*
> +*@param addr   Address of the memory region
> +*@param size    Size of the memory region
> +*
> +*@return       Buffer Handle to the newly create odp buffer
> +*/
> +odp_buffer_t     odp_buffer_from_memory_region(void* addr, int size );
> +
> +/**
> +* Merge Buffer list into a singe buffer
> +*@param buf    Array of ODP Buffer Handles
> +*@param num    Number of Buffers in the buf Array
> +
> +*@return       Buffer Handle to the newly created odp buffer
> +**/
> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
> +
> +/**
> +* Split a single Buffer into a list of odp buffers
> +*
> +*@param buf    Buffer Handle
> +*
> +*@param        size    Maximum size of the individual buffers
> +*
> +*@return       Array of newly created Buffer Handle
> +**/
> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
> +
> +/**
> +* Append a buffer at the end of a scatter-gather list of buffer
> +*
> +*@param buf    Buffer Handle to the scatter-gather list of buffer
> +*
> +*@param tail   Buffer Handle to the buffer which is appended
> +*
> +*@return       1 if success 0 otherwise
> +**/
> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
> +
> +/**
> +* Add a buffer at the head of a existing Scatter-gather Buffer
> +*
> +*@param buf    Buffer Handle to the scatter-gather list of buffer
> +*
> +*@param head   Buffer Handle to the buffer which is added at the head
> +*
> +*@return       1 if success 0 otherwise
> +**/
> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>
>  #ifdef __cplusplus
>  }
> --
> 1.7.9.5
>
>

Hi,


On Tue, Jun 17, 2014 at 2:51 AM, Bala Manoharan <bala.manoharan@linaro.org>
wrote:

> Hi All,
>
> This patch is meant for discussion on scatter-gather buffer support.
> The implementation of these functions will be submitted once the proposed
> APIs are finalised.
>
> Regards,
> Bala
>
>
> On 16 June 2014 16:11, <bala.manoharan@linaro.org> wrote:
>
>> From: Bala Manoharan <bala.manoharan@linaro.org>
>>
>> This patch contains the initial draft of Buffer Management with support
>> for scatter-gather list
>>
>> Regards,
>> Bala
>>
>> Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
>> ---
>>  include/odp_buffer.h |   73
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>
>> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
>> index d79e76d..c313168 100644
>> --- a/include/odp_buffer.h
>> +++ b/include/odp_buffer.h
>> @@ -92,6 +92,79 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>>   */
>>  void odp_buffer_print(odp_buffer_t buf);
>>
>> +/**
>> +*Number of segments in this scatter-gather List
>> +*
>> +*@param buf    Buffer Handle
>> +*
>> +*@return       Total number of segments in this scatter-gather List.
>> +*
>> +**/
>> +int odp_buffer_segment_count(odp_buffer_t buf)
>> +
>> +/**
>> +*Address of the segment in the buffer list.
>> +*
>> +*@param buf    Buffer Handle
>> +*@param n      Index number for the segment in the SG list
>> +*@param size   Returns the size of the current addressable segment
>> +
>> +*@return       Segment start address
>> +**/
>> +
>> +void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
>> +
>> +/*
>> +*Creates ODP Buffer from a memory region
>> +*
>> +*@param addr   Address of the memory region
>> +*@param size    Size of the memory region
>> +*
>> +*@return       Buffer Handle to the newly create odp buffer
>> +*/
>> +odp_buffer_t     odp_buffer_from_memory_region(void* addr, int size );
>>
>
We should keep consistent with buffer sizes, odp_buffer_size for instance
returns a size_t. Also, does this assume that the data will be copied, so
that the user can safely free the memory area pointed to by addr?

+
>> +/**
>> +* Merge Buffer list into a singe buffer
>> +*@param buf    Array of ODP Buffer Handles
>> +*@param num    Number of Buffers in the buf Array
>> +
>> +*@return       Buffer Handle to the newly created odp buffer
>> +**/
>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>> +
>>
>
Which pool should the resulting buffer be allocated from? Will there be an
SG list in the resulting buffer or just one big chunk?


> +/**
>> +* Split a single Buffer into a list of odp buffers
>> +*
>> +*@param buf    Buffer Handle
>> +*
>> +*@param        size    Maximum size of the individual buffers
>> +*
>> +*@return       Array of newly created Buffer Handle
>> +**/
>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>
>
This doesn't seem entirely related to SG support. Is there zero-copy here?

> +
>> +/**
>> +* Append a buffer at the end of a scatter-gather list of buffer
>> +*
>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>> +*
>> +*@param tail   Buffer Handle to the buffer which is appended
>> +*
>> +*@return       1 if success 0 otherwise
>> +**/
>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>
> +
>> +/**
>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>> +*
>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>> +*
>> +*@param head   Buffer Handle to the buffer which is added at the head
>> +*
>> +*@return       1 if success 0 otherwise
>> +**/
>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>
>
It's not clear to me whether we should support scatter-gather lists made
out of odp buffers or random memory. Surely it is easier to work with odp
buffers only, but that may not be enough for our purposes.

/Ciprian


>>  #ifdef __cplusplus
>>  }
>> --
>> 1.7.9.5
>>
>>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>

Hi Ciprian,

Pls find my reply inline

We should keep consistent with buffer sizes, odp_buffer_size for instance
returns a size_t. Also, does this assume that the data will be copied, so
that the user can safely free the memory area pointed to by addr?

[Bala] Agreed. The return should be size_t. The data will be associated
with the Buffer Handle returned by the function. Hence it will get freed
when the buffer Handle is freed.

> +
>> +/**
>> +* Merge Buffer list into a singe buffer
>> +*@param buf    Array of ODP Buffer Handles
>> +*@param num    Number of Buffers in the buf Array
>> +
>> +*@return       Buffer Handle to the newly created odp buffer
>> +**/
>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>> +
>>
>
Which pool should the resulting buffer be allocated from? Will there be an
SG list in the resulting buffer or just one big chunk?
 [Bala] During Merging the individual buffers will be converted into a SG
list. There will not be any requirement for a buffer pool.

 +/**
>> +* Split a single Buffer into a list of odp buffers
>> +*
>> +*@param buf    Buffer Handle
>> +*
>> +*@param        size    Maximum size of the individual buffers
>> +*
>> +*@return       Array of newly created Buffer Handle
>> +**/
>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>
>
This doesn't seem entirely related to SG support. Is there zero-copy here?
[Bala]  This requirement is to support the scenario for fragmenting a
packet.
Ideally this should be implemented using zero-copy but we can get opinion
of all the HW vendors and finalize the same.

>  +
>> +/**
>> +* Append a buffer at the end of a scatter-gather list of buffer
>> +*
>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>> +*
>> +*@param tail   Buffer Handle to the buffer which is appended
>> +*
>> +*@return       1 if success 0 otherwise
>> +**/
>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>
>  +
>> +/**
>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>> +*
>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>> +*
>> +*@param head   Buffer Handle to the buffer which is added at the head
>> +*
>> +*@return       1 if success 0 otherwise
>> +**/
>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>
>
It's not clear to me whether we should support scatter-gather lists made
out of odp buffers or random memory. Surely it is easier to work with odp
buffers only, but that may not be enough for our purposes.
[Bala]  Scatter-gather lists were considered to be made up of odp buffers.
If you have any additional requirements or suggestions pls provide.


Regards,
Bala


On 19 June 2014 02:20, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:

> Hi,
>
>
> On Tue, Jun 17, 2014 at 2:51 AM, Bala Manoharan <bala.manoharan@linaro.org
> > wrote:
>
>> Hi All,
>>
>> This patch is meant for discussion on scatter-gather buffer support.
>> The implementation of these functions will be submitted once the proposed
>> APIs are finalised.
>>
>> Regards,
>> Bala
>>
>>
>> On 16 June 2014 16:11, <bala.manoharan@linaro.org> wrote:
>>
>>> From: Bala Manoharan <bala.manoharan@linaro.org>
>>>
>>> This patch contains the initial draft of Buffer Management with support
>>> for scatter-gather list
>>>
>>> Regards,
>>> Bala
>>>
>>> Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
>>> ---
>>>  include/odp_buffer.h |   73
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 73 insertions(+)
>>>
>>> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
>>> index d79e76d..c313168 100644
>>> --- a/include/odp_buffer.h
>>> +++ b/include/odp_buffer.h
>>> @@ -92,6 +92,79 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>>>   */
>>>  void odp_buffer_print(odp_buffer_t buf);
>>>
>>> +/**
>>> +*Number of segments in this scatter-gather List
>>> +*
>>> +*@param buf    Buffer Handle
>>> +*
>>> +*@return       Total number of segments in this scatter-gather List.
>>> +*
>>> +**/
>>> +int odp_buffer_segment_count(odp_buffer_t buf)
>>> +
>>> +/**
>>> +*Address of the segment in the buffer list.
>>> +*
>>> +*@param buf    Buffer Handle
>>> +*@param n      Index number for the segment in the SG list
>>> +*@param size   Returns the size of the current addressable segment
>>> +
>>> +*@return       Segment start address
>>> +**/
>>> +
>>> +void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
>>> +
>>> +/*
>>> +*Creates ODP Buffer from a memory region
>>> +*
>>> +*@param addr   Address of the memory region
>>> +*@param size    Size of the memory region
>>> +*
>>> +*@return       Buffer Handle to the newly create odp buffer
>>> +*/
>>> +odp_buffer_t     odp_buffer_from_memory_region(void* addr, int size );
>>>
>>
> We should keep consistent with buffer sizes, odp_buffer_size for instance
> returns a size_t. Also, does this assume that the data will be copied, so
> that the user can safely free the memory area pointed to by addr?
>
> +
>>> +/**
>>> +* Merge Buffer list into a singe buffer
>>> +*@param buf    Array of ODP Buffer Handles
>>> +*@param num    Number of Buffers in the buf Array
>>> +
>>> +*@return       Buffer Handle to the newly created odp buffer
>>> +**/
>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>> +
>>>
>>
> Which pool should the resulting buffer be allocated from? Will there be an
> SG list in the resulting buffer or just one big chunk?
>
>
>> +/**
>>> +* Split a single Buffer into a list of odp buffers
>>> +*
>>> +*@param buf    Buffer Handle
>>> +*
>>> +*@param        size    Maximum size of the individual buffers
>>> +*
>>> +*@return       Array of newly created Buffer Handle
>>> +**/
>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>
>>
> This doesn't seem entirely related to SG support. Is there zero-copy here?
>
>>  +
>>> +/**
>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param tail   Buffer Handle to the buffer which is appended
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>
>>  +
>>> +/**
>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>
>>
> It's not clear to me whether we should support scatter-gather lists made
> out of odp buffers or random memory. Surely it is easier to work with odp
> buffers only, but that may not be enough for our purposes.
>
> /Ciprian
>
>
>>>  #ifdef __cplusplus
>>>  }
>>> --
>>> 1.7.9.5
>>>
>>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>

On Thu, Jun 19, 2014 at 7:32 PM, Bala Manoharan <bala.manoharan@linaro.org>
wrote:

> Hi Ciprian,
>
> Pls find my reply inline
>
>
> We should keep consistent with buffer sizes, odp_buffer_size for instance
> returns a size_t. Also, does this assume that the data will be copied, so
> that the user can safely free the memory area pointed to by addr?
>
> [Bala] Agreed. The return should be size_t. The data will be associated
> with the Buffer Handle returned by the function. Hence it will get freed
> when the buffer Handle is freed.
>

Just a clarification, the parameter size should pe of type size_t.
About freeing the memory, if the memory region passed as parameter is not
expected to be of some type, then freeing it (when the resulting buffer is
not needed anymore) should be done through a callback, because only the
user knows how it was allocated (if it was even allocated). This also
introduces problems with ownership and process context.

 +
>>> +/**
>>> +* Merge Buffer list into a singe buffer
>>> +*@param buf    Array of ODP Buffer Handles
>>> +*@param num    Number of Buffers in the buf Array
>>> +
>>> +*@return       Buffer Handle to the newly created odp buffer
>>> +**/
>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>> +
>>>
>>
> Which pool should the resulting buffer be allocated from? Will there be an
> SG list in the resulting buffer or just one big chunk?
>  [Bala] During Merging the individual buffers will be converted into a SG
> list. There will not be any requirement for a buffer pool.
>

Yes but you still need to allocate the odp_buffer_t from somewhere, the
metadata of the buffer has to exist somewhere even if the payload is
referenced through a pointer.

>
>  +/**
>>> +* Split a single Buffer into a list of odp buffers
>>> +*
>>> +*@param buf    Buffer Handle
>>> +*
>>> +*@param        size    Maximum size of the individual buffers
>>> +*
>>> +*@return       Array of newly created Buffer Handle
>>> +**/
>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>
>>
> This doesn't seem entirely related to SG support. Is there zero-copy here?
> [Bala]  This requirement is to support the scenario for fragmenting a
> packet.
> Ideally this should be implemented using zero-copy but we can get opinion
> of all the HW vendors and finalize the same.
>

If this results in a buffer that contains an SG list, then it would be
better to return the handle to that buffer, rather than an array of
buffers. Is that the intent?


>  +
>>> +/**
>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param tail   Buffer Handle to the buffer which is appended
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>
>>  +
>>> +/**
>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>
>>
> It's not clear to me whether we should support scatter-gather lists made
> out of odp buffers or random memory. Surely it is easier to work with odp
> buffers only, but that may not be enough for our purposes.
> [Bala]  Scatter-gather lists were considered to be made up of odp buffers.
> If you have any additional requirements or suggestions pls provide.
>

It wasn't clear to me that the SG fragments are made up of odp buffers.
Even if they are random memory that are pointed to by an odp buffer, I
think it would still work better to return the buffer, rather than a
pointer like it is the case with odp_buffer_segment_addr. This function
should in fact return the nth buffer in a SG odp buffer and additionally
introduce an API (if there isn't already one that we can use for that) for
getting a pointer to the payload from a buffer. It would make it clear IMO
that SG support is constructed over odp buffers.

/Ciprian

>
>
> Regards,
> Bala
>
>
> On 19 June 2014 02:20, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:
>
>> Hi,
>>
>>
>> On Tue, Jun 17, 2014 at 2:51 AM, Bala Manoharan <
>> bala.manoharan@linaro.org> wrote:
>>
>>> Hi All,
>>>
>>> This patch is meant for discussion on scatter-gather buffer support.
>>> The implementation of these functions will be submitted once the
>>> proposed APIs are finalised.
>>>
>>> Regards,
>>> Bala
>>>
>>>
>>> On 16 June 2014 16:11, <bala.manoharan@linaro.org> wrote:
>>>
>>>> From: Bala Manoharan <bala.manoharan@linaro.org>
>>>>
>>>> This patch contains the initial draft of Buffer Management with support
>>>> for scatter-gather list
>>>>
>>>> Regards,
>>>> Bala
>>>>
>>>> Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
>>>> ---
>>>>  include/odp_buffer.h |   73
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
>>>> index d79e76d..c313168 100644
>>>> --- a/include/odp_buffer.h
>>>> +++ b/include/odp_buffer.h
>>>> @@ -92,6 +92,79 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>>>>   */
>>>>  void odp_buffer_print(odp_buffer_t buf);
>>>>
>>>> +/**
>>>> +*Number of segments in this scatter-gather List
>>>> +*
>>>> +*@param buf    Buffer Handle
>>>> +*
>>>> +*@return       Total number of segments in this scatter-gather List.
>>>> +*
>>>> +**/
>>>> +int odp_buffer_segment_count(odp_buffer_t buf)
>>>> +
>>>> +/**
>>>> +*Address of the segment in the buffer list.
>>>> +*
>>>> +*@param buf    Buffer Handle
>>>> +*@param n      Index number for the segment in the SG list
>>>> +*@param size   Returns the size of the current addressable segment
>>>> +
>>>> +*@return       Segment start address
>>>> +**/
>>>> +
>>>> +void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
>>>> +
>>>> +/*
>>>> +*Creates ODP Buffer from a memory region
>>>> +*
>>>> +*@param addr   Address of the memory region
>>>> +*@param size    Size of the memory region
>>>> +*
>>>> +*@return       Buffer Handle to the newly create odp buffer
>>>> +*/
>>>> +odp_buffer_t     odp_buffer_from_memory_region(void* addr, int size );
>>>>
>>>
>> We should keep consistent with buffer sizes, odp_buffer_size for instance
>> returns a size_t. Also, does this assume that the data will be copied, so
>> that the user can safely free the memory area pointed to by addr?
>>
>> +
>>>> +/**
>>>> +* Merge Buffer list into a singe buffer
>>>> +*@param buf    Array of ODP Buffer Handles
>>>> +*@param num    Number of Buffers in the buf Array
>>>> +
>>>> +*@return       Buffer Handle to the newly created odp buffer
>>>> +**/
>>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>>> +
>>>>
>>>
>> Which pool should the resulting buffer be allocated from? Will there be
>> an SG list in the resulting buffer or just one big chunk?
>>
>>
>>> +/**
>>>> +* Split a single Buffer into a list of odp buffers
>>>> +*
>>>> +*@param buf    Buffer Handle
>>>> +*
>>>> +*@param        size    Maximum size of the individual buffers
>>>> +*
>>>> +*@return       Array of newly created Buffer Handle
>>>> +**/
>>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>>
>>>
>> This doesn't seem entirely related to SG support. Is there zero-copy here?
>>
>>>  +
>>>> +/**
>>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>>> +*
>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>> +*
>>>> +*@param tail   Buffer Handle to the buffer which is appended
>>>> +*
>>>> +*@return       1 if success 0 otherwise
>>>> +**/
>>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>>
>>>  +
>>>> +/**
>>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>>> +*
>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>> +*
>>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>>> +*
>>>> +*@return       1 if success 0 otherwise
>>>> +**/
>>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>>
>>>
>> It's not clear to me whether we should support scatter-gather lists made
>> out of odp buffers or random memory. Surely it is easier to work with odp
>> buffers only, but that may not be enough for our purposes.
>>
>> /Ciprian
>>
>>
>>>>  #ifdef __cplusplus
>>>>  }
>>>> --
>>>> 1.7.9.5
>>>>
>>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>>
>>
>

Hi,

Reply inline.

Regards,
Bala

Hi Ciprian,
>
> Pls find my reply inline
>
>
> We should keep consistent with buffer sizes, odp_buffer_size for instance
> returns a size_t. Also, does this assume that the data will be copied, so
> that the user can safely free the memory area pointed to by addr?
>
> [Bala] Agreed. The return should be size_t. The data will be associated
> with the Buffer Handle returned by the function. Hence it will get freed
> when the buffer Handle is freed.
>

Just a clarification, the parameter size should pe of type size_t.
About freeing the memory, if the memory region passed as parameter is not
expected to be of some type, then freeing it (when the resulting buffer is
not needed anymore) should be done through a callback, because only the
user knows how it was allocated (if it was even allocated). This also
introduces problems with ownership and process context.

[Bala] Callback mechanism is not supported in ODP. Once the user has
provided the memory region to be converted into a odp_buffer_t, the
ownership of the memory region should be with the implementation rather
than the application as otherwise it creates an issue of how to initiate
the control back to the application once the memory gets freed.
The implementation can create a buffer pool with a single buffer in the
pool out of the given memory region and allocate the odp_buffer_t from it.
 This is my opinion and I am open to other view points.

>  +
>>> +/**
>>> +* Merge Buffer list into a singe buffer
>>> +*@param buf    Array of ODP Buffer Handles
>>> +*@param num    Number of Buffers in the buf Array
>>> +
>>> +*@return       Buffer Handle to the newly created odp buffer
>>> +**/
>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>> +
>>>
>>
> Which pool should the resulting buffer be allocated from? Will there be an
> SG list in the resulting buffer or just one big chunk?
>  [Bala] During Merging the individual buffers will be converted into a SG
> list. There will not be any requirement for a buffer pool.
>

Yes but you still need to allocate the odp_buffer_t from somewhere, the
metadata of the buffer has to exist somewhere even if the payload is
referenced through a pointer.
[Bala] The meta-data of the buffer is kept hidden from the application and
the implementation can use any one of the given odp_buffer_t metadata to
fill the information about the remaining odp segments. Basically the
implementation can convert a single odp_buffer_t meta-data to contain the
information of the SG list. zero-copy merging of the buffer will be the
preferred method as there will not be any additional delay involved.

>
>  +/**
>>> +* Split a single Buffer into a list of odp buffers
>>> +*
>>> +*@param buf    Buffer Handle
>>> +*
>>> +*@param        size    Maximum size of the individual buffers
>>> +*
>>> +*@return       Array of newly created Buffer Handle
>>> +**/
>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>
>>
> This doesn't seem entirely related to SG support. Is there zero-copy here?
> [Bala]  This requirement is to support the scenario for fragmenting a
> packet.
> Ideally this should be implemented using zero-copy but we can get opinion
> of all the HW vendors and finalize the same.
>

If this results in a buffer that contains an SG list, then it would be
better to return the handle to that buffer, rather than an array of
buffers. Is that the intent?
[Bala] The result of this operation is creation of multiple odp_buffer_t
handles. The idea was to create multiple buffers from the same pool from
which the input buffer was available and create multiple smaller buffers.
If there is a use-case to create multiple buffers from a pool which is
different than the pool of the input buffer, we can add the pool
information as input parameter and create a default value which will be the
same pool as the input buffer.



>  +
>>> +/**
>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param tail   Buffer Handle to the buffer which is appended
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>
>>  +
>>> +/**
>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>> +*
>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>> +*
>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>> +*
>>> +*@return       1 if success 0 otherwise
>>> +**/
>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>
>>
> It's not clear to me whether we should support scatter-gather lists made
> out of odp buffers or random memory. Surely it is easier to work with odp
> buffers only, but that may not be enough for our purposes.
> [Bala]  Scatter-gather lists were considered to be made up of odp buffers.
> If you have any additional requirements or suggestions pls provide.
>

It wasn't clear to me that the SG fragments are made up of odp buffers.
Even if they are random memory that are pointed to by an odp buffer, I
think it would still work better to return the buffer, rather than a
pointer like it is the case with odp_buffer_segment_addr. This function
should in fact return the nth buffer in a SG odp buffer and additionally
introduce an API (if there isn't already one that we can use for that) for
getting a pointer to the payload from a buffer. It would make it clear IMO
that SG support is constructed over odp buffers.

[Bala] The idea behind this API proposal is to hide the internal details of
the SG list from the application which is a general use-case as the
application does not need to know about the header information within each
of the SG segments but it only needs to know the meta-data in the first
buffer. This proposal maps well with the HWs since the Scatter-gather list
is created from the HW and usually the application does not care about the
internals of each segment header. The application only needs to know the
valid start address and the size of the buffer for the next segment.
If we introduce odp_buffer_t header for each segment of SG list then there
will be additional information stored per segment which is not needed.

These APIs maps well for use-case where the implementation stores the
scatter-gather as a list of complete odp_buffer_t as in those case also the
implementation can manipulate the meta-data and provide the application
with the start of the valid address in each segment and the size which can
be written in each segment.

Regards,
Bala


On 20 June 2014 02:42, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:

>
>
>
> On Thu, Jun 19, 2014 at 7:32 PM, Bala Manoharan <bala.manoharan@linaro.org
> > wrote:
>
>> Hi Ciprian,
>>
>> Pls find my reply inline
>>
>>
>> We should keep consistent with buffer sizes, odp_buffer_size for instance
>> returns a size_t. Also, does this assume that the data will be copied, so
>> that the user can safely free the memory area pointed to by addr?
>>
>> [Bala] Agreed. The return should be size_t. The data will be associated
>> with the Buffer Handle returned by the function. Hence it will get freed
>> when the buffer Handle is freed.
>>
>
> Just a clarification, the parameter size should pe of type size_t.
> About freeing the memory, if the memory region passed as parameter is not
> expected to be of some type, then freeing it (when the resulting buffer is
> not needed anymore) should be done through a callback, because only the
> user knows how it was allocated (if it was even allocated). This also
> introduces problems with ownership and process context.
>
>  +
>>>> +/**
>>>> +* Merge Buffer list into a singe buffer
>>>> +*@param buf    Array of ODP Buffer Handles
>>>> +*@param num    Number of Buffers in the buf Array
>>>> +
>>>> +*@return       Buffer Handle to the newly created odp buffer
>>>> +**/
>>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>>> +
>>>>
>>>
>> Which pool should the resulting buffer be allocated from? Will there be
>> an SG list in the resulting buffer or just one big chunk?
>>  [Bala] During Merging the individual buffers will be converted into a SG
>> list. There will not be any requirement for a buffer pool.
>>
>
> Yes but you still need to allocate the odp_buffer_t from somewhere, the
> metadata of the buffer has to exist somewhere even if the payload is
> referenced through a pointer.
>
>>
>>  +/**
>>>> +* Split a single Buffer into a list of odp buffers
>>>> +*
>>>> +*@param buf    Buffer Handle
>>>> +*
>>>> +*@param        size    Maximum size of the individual buffers
>>>> +*
>>>> +*@return       Array of newly created Buffer Handle
>>>> +**/
>>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>>
>>>
>> This doesn't seem entirely related to SG support. Is there zero-copy here?
>> [Bala]  This requirement is to support the scenario for fragmenting a
>> packet.
>> Ideally this should be implemented using zero-copy but we can get opinion
>> of all the HW vendors and finalize the same.
>>
>
> If this results in a buffer that contains an SG list, then it would be
> better to return the handle to that buffer, rather than an array of
> buffers. Is that the intent?
>
>
>>  +
>>>> +/**
>>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>>> +*
>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>> +*
>>>> +*@param tail   Buffer Handle to the buffer which is appended
>>>> +*
>>>> +*@return       1 if success 0 otherwise
>>>> +**/
>>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>>
>>>  +
>>>> +/**
>>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>>> +*
>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>> +*
>>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>>> +*
>>>> +*@return       1 if success 0 otherwise
>>>> +**/
>>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>>
>>>
>> It's not clear to me whether we should support scatter-gather lists made
>> out of odp buffers or random memory. Surely it is easier to work with odp
>> buffers only, but that may not be enough for our purposes.
>> [Bala]  Scatter-gather lists were considered to be made up of odp
>> buffers. If you have any additional requirements or suggestions pls provide.
>>
>
> It wasn't clear to me that the SG fragments are made up of odp buffers.
> Even if they are random memory that are pointed to by an odp buffer, I
> think it would still work better to return the buffer, rather than a
> pointer like it is the case with odp_buffer_segment_addr. This function
> should in fact return the nth buffer in a SG odp buffer and additionally
> introduce an API (if there isn't already one that we can use for that) for
> getting a pointer to the payload from a buffer. It would make it clear IMO
> that SG support is constructed over odp buffers.
>
> /Ciprian
>
>>
>>
>> Regards,
>> Bala
>>
>>
>> On 19 June 2014 02:20, Ciprian Barbu <ciprian.barbu@linaro.org> wrote:
>>
>>> Hi,
>>>
>>>
>>> On Tue, Jun 17, 2014 at 2:51 AM, Bala Manoharan <
>>> bala.manoharan@linaro.org> wrote:
>>>
>>>> Hi All,
>>>>
>>>> This patch is meant for discussion on scatter-gather buffer support.
>>>> The implementation of these functions will be submitted once the
>>>> proposed APIs are finalised.
>>>>
>>>> Regards,
>>>> Bala
>>>>
>>>>
>>>> On 16 June 2014 16:11, <bala.manoharan@linaro.org> wrote:
>>>>
>>>>> From: Bala Manoharan <bala.manoharan@linaro.org>
>>>>>
>>>>> This patch contains the initial draft of Buffer Management with
>>>>> support for scatter-gather list
>>>>>
>>>>> Regards,
>>>>> Bala
>>>>>
>>>>> Signed-off-by: Bala Manoharan <bala.manoharan@linaro.org>
>>>>> ---
>>>>>  include/odp_buffer.h |   73
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 73 insertions(+)
>>>>>
>>>>> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
>>>>> index d79e76d..c313168 100644
>>>>> --- a/include/odp_buffer.h
>>>>> +++ b/include/odp_buffer.h
>>>>> @@ -92,6 +92,79 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>>>>>   */
>>>>>  void odp_buffer_print(odp_buffer_t buf);
>>>>>
>>>>> +/**
>>>>> +*Number of segments in this scatter-gather List
>>>>> +*
>>>>> +*@param buf    Buffer Handle
>>>>> +*
>>>>> +*@return       Total number of segments in this scatter-gather List.
>>>>> +*
>>>>> +**/
>>>>> +int odp_buffer_segment_count(odp_buffer_t buf)
>>>>> +
>>>>> +/**
>>>>> +*Address of the segment in the buffer list.
>>>>> +*
>>>>> +*@param buf    Buffer Handle
>>>>> +*@param n      Index number for the segment in the SG list
>>>>> +*@param size   Returns the size of the current addressable segment
>>>>> +
>>>>> +*@return       Segment start address
>>>>> +**/
>>>>> +
>>>>> +void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
>>>>> +
>>>>> +/*
>>>>> +*Creates ODP Buffer from a memory region
>>>>> +*
>>>>> +*@param addr   Address of the memory region
>>>>> +*@param size    Size of the memory region
>>>>> +*
>>>>> +*@return       Buffer Handle to the newly create odp buffer
>>>>> +*/
>>>>> +odp_buffer_t     odp_buffer_from_memory_region(void* addr, int size );
>>>>>
>>>>
>>> We should keep consistent with buffer sizes, odp_buffer_size for
>>> instance returns a size_t. Also, does this assume that the data will be
>>> copied, so that the user can safely free the memory area pointed to by addr?
>>>
>>> +
>>>>> +/**
>>>>> +* Merge Buffer list into a singe buffer
>>>>> +*@param buf    Array of ODP Buffer Handles
>>>>> +*@param num    Number of Buffers in the buf Array
>>>>> +
>>>>> +*@return       Buffer Handle to the newly created odp buffer
>>>>> +**/
>>>>> +odp_buffer_t    odp_buffer_merge(odp_buffer_t[] buf, int num);
>>>>> +
>>>>>
>>>>
>>> Which pool should the resulting buffer be allocated from? Will there be
>>> an SG list in the resulting buffer or just one big chunk?
>>>
>>>
>>>> +/**
>>>>> +* Split a single Buffer into a list of odp buffers
>>>>> +*
>>>>> +*@param buf    Buffer Handle
>>>>> +*
>>>>> +*@param        size    Maximum size of the individual buffers
>>>>> +*
>>>>> +*@return       Array of newly created Buffer Handle
>>>>> +**/
>>>>> +odp_buffer_t[] odp_buffer_split(odp_buffer_t buf, int size);
>>>>>
>>>>
>>> This doesn't seem entirely related to SG support. Is there zero-copy
>>> here?
>>>
>>>>  +
>>>>> +/**
>>>>> +* Append a buffer at the end of a scatter-gather list of buffer
>>>>> +*
>>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>>> +*
>>>>> +*@param tail   Buffer Handle to the buffer which is appended
>>>>> +*
>>>>> +*@return       1 if success 0 otherwise
>>>>> +**/
>>>>> +int odp_buffer_append(odp_buffer_t buf, odp_buffer_t tail );
>>>>
>>>>  +
>>>>> +/**
>>>>> +* Add a buffer at the head of a existing Scatter-gather Buffer
>>>>> +*
>>>>> +*@param buf    Buffer Handle to the scatter-gather list of buffer
>>>>> +*
>>>>> +*@param head   Buffer Handle to the buffer which is added at the head
>>>>> +*
>>>>> +*@return       1 if success 0 otherwise
>>>>> +**/
>>>>> +int odp_buffer_insert_at_head(odp_buffer_t buf, odp_buffer_t head);
>>>>>
>>>>
>>> It's not clear to me whether we should support scatter-gather lists made
>>> out of odp buffers or random memory. Surely it is easier to work with odp
>>> buffers only, but that may not be enough for our purposes.
>>>
>>> /Ciprian
>>>
>>>
>>>>>  #ifdef __cplusplus
>>>>>  }
>>>>> --
>>>>> 1.7.9.5
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> lng-odp mailing list
>>>> lng-odp@lists.linaro.org
>>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>>
>>>>
>>>
>>
>