Packet segment API started

Started buffer/packet/SG-list API harmonizartion. New API terminology
is introduced first in segments (SG list) API and buffer/packet APIs
are changed to use same terms in following patches. Segments are
introduced for packet type buffers only at this phase.

Term summary:
- xxx_addr     = segment start address
- xxx_size     = segment size
- xxx_data     = data pointer (data start inside the segment)
- xxx_data_len = data length

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
---
 include/helper/odp_packet_helper.h              |  14 ---
 include/odp_buffer.h                            |  11 --
 include/odp_buffer_pool.h                       |   9 +-
 include/odp_packet.h                            | 148 ++++++++++++++++++++++++
 platform/linux-generic/source/odp_buffer.c      |  11 --
 platform/linux-generic/source/odp_buffer_pool.c |   9 ++
 platform/linux-generic/source/odp_packet.c      |  20 ++++
 7 files changed, 185 insertions(+), 37 deletions(-)

Applied, thanks!

Maxim.

On 07/01/2014 12:08 PM, Petri Savolainen wrote:
> Started buffer/packet/SG-list API harmonizartion. New API terminology
> is introduced first in segments (SG list) API and buffer/packet APIs
> are changed to use same terms in following patches. Segments are
> introduced for packet type buffers only at this phase.
>
> Term summary:
> - xxx_addr     = segment start address
> - xxx_size     = segment size
> - xxx_data     = data pointer (data start inside the segment)
> - xxx_data_len = data length
>
> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> ---
>   include/helper/odp_packet_helper.h              |  14 ---
>   include/odp_buffer.h                            |  11 --
>   include/odp_buffer_pool.h                       |   9 +-
>   include/odp_packet.h                            | 148 ++++++++++++++++++++++++
>   platform/linux-generic/source/odp_buffer.c      |  11 --
>   platform/linux-generic/source/odp_buffer_pool.c |   9 ++
>   platform/linux-generic/source/odp_packet.c      |  20 ++++
>   7 files changed, 185 insertions(+), 37 deletions(-)
>
> diff --git a/include/helper/odp_packet_helper.h b/include/helper/odp_packet_helper.h
> index eed866e..db12028 100644
> --- a/include/helper/odp_packet_helper.h
> +++ b/include/helper/odp_packet_helper.h
> @@ -89,20 +89,6 @@ static inline size_t odp_packet_buf_size(odp_packet_t pkt)
>   	return odp_buffer_size(buf);
>   }
>   
> -/**
> - * Helper: Tests if packet is part of a scatter/gather list
> - *
> - * @param pkt  Packet handle
> - *
> - * @return 1 if belongs to a scatter list, otherwise 0
> - */
> -static inline int odp_packet_is_scatter(odp_packet_t pkt)
> -{
> -	odp_buffer_t buf = odp_buffer_from_packet(pkt);
> -
> -	return odp_buffer_is_scatter(buf);
> -}
> -
>   
>   #ifdef __cplusplus
>   }
> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
> index b3d6f4a..d8577fd 100644
> --- a/include/odp_buffer.h
> +++ b/include/odp_buffer.h
> @@ -19,13 +19,10 @@ extern "C" {
>   #endif
>   
>   
> -
>   #include <odp_std_types.h>
>   
>   
>   
> -
> -
>   /**
>    * ODP buffer
>    */
> @@ -68,14 +65,6 @@ int odp_buffer_type(odp_buffer_t buf);
>   #define ODP_BUFFER_TYPE_PACKET    2  /**< Packet buffer */
>   #define ODP_BUFFER_TYPE_TIMEOUT   3  /**< Timeout buffer */
>   
> -/**
> - * Tests if buffer is part of a scatter/gather list
> - *
> - * @param buf      Buffer handle
> - *
> - * @return 1 if belongs to a scatter list, otherwise 0
> - */
> -int odp_buffer_is_scatter(odp_buffer_t buf);
>   
>   /**
>    * Tests if buffer is valid
> diff --git a/include/odp_buffer_pool.h b/include/odp_buffer_pool.h
> index 9112489..69155cc 100644
> --- a/include/odp_buffer_pool.h
> +++ b/include/odp_buffer_pool.h
> @@ -70,7 +70,6 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name);
>   void odp_buffer_pool_print(odp_buffer_pool_t pool);
>   
>   
> -
>   /**
>    * Buffer alloc
>    *
> @@ -90,6 +89,14 @@ odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool);
>   void odp_buffer_free(odp_buffer_t buf);
>   
>   
> +/**
> + * Buffer pool of the buffer
> + *
> + * @param buf       Buffer handle
> + *
> + * @return Buffer pool the buffer was allocated from
> + */
> +odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
>   
>   
>   #ifdef __cplusplus
> diff --git a/include/odp_packet.h b/include/odp_packet.h
> index 59759bb..8ae6410 100644
> --- a/include/odp_packet.h
> +++ b/include/odp_packet.h
> @@ -220,6 +220,154 @@ void odp_packet_print(odp_packet_t pkt);
>    */
>   int odp_packet_copy(odp_packet_t pkt_dst, odp_packet_t pkt_src);
>   
> +/**
> + * Tests if packet is segmented (a scatter/gather list)
> + *
> + * @param pkt  Packet handle
> + *
> + * @return Non-zero if packet is segmented, otherwise 0
> + */
> +int odp_packet_is_segmented(odp_packet_t pkt);
> +
> +/**
> + * Segment count
> + *
> + * Returns number of segments in the packet. A packet has always at least one
> + * segment (the packet buffer itself).
> + *
> + * @param pkt  Packet handle
> + *
> + * @return Segment count
> + */
> +int odp_packet_seg_count(odp_packet_t pkt);
> +
> +/**
> + * Segment start address
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment start address
> + */
> +void *odp_packet_seg_addr(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment maximum data size
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment maximum data size
> + */
> +size_t odp_packet_seg_size(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment data address
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment data address
> + */
> +void *odp_packet_seg_data(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment data length
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment data length
> + */
> +size_t odp_packet_seg_data_len(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment headroom
> + *
> + * seg_headroom = seg_data - seg_addr
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Number of octets from seg_addr to seg_data
> + */
> +size_t odp_packet_seg_headroom(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment tailroom
> + *
> + * seg_tailroom = seg_size - seg_headroom - seg_data_len
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Number of octets from end-of-data to end-of-segment
> + */
> +size_t odp_packet_seg_tailroom(odp_packet_t pkt, int seg);
> +
> +/**
> + * Push out segment head
> + *
> + * Push out segment data address (away from data) and increase data length.
> + *
> + * seg_data     -= len
> + * seg_data_len += len
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + * @param len  Number of octets to push head (0 ... seg_headroom)
> + *
> + * @return New segment data address, or NULL on an error
> + */
> +void *odp_packet_seg_push_head(odp_packet_t pkt, int seg, size_t len);
> +
> +/**
> + * Pull in segment head
> + *
> + * Pull in segment data address (towards data) and decrease data length.
> + *
> + * seg_data     += len
> + * seg_data_len -= len
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + * @param len  Number of octets to pull head (0 ... seg_data_len)
> + *
> + * @return New segment data address, or NULL on an error
> + */
> +void *odp_packet_seg_pull_head(odp_packet_t pkt, int seg, size_t len);
> +
> +/**
> + * Push out segment tail
> + *
> + * Increase segment data length.
> + *
> + * seg_data_len  += len
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + * @param len  Number of octets to push tail (0 ... seg_tailroom)
> + *
> + * @return New segment data length, or -1 on an error
> + */
> +int odp_packet_seg_push_tail(odp_packet_t pkt, int seg, size_t len);
> +
> +/**
> + * Pull in segment tail
> + *
> + * Decrease segment data length.
> + *
> + * seg_data_len  -= len
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + * @param len  Number of octets to pull tail (0 ... seg_data_len)
> + *
> + * @return New segment data length, or -1 on an error
> + */
> +int odp_packet_seg_pull_tail(odp_packet_t pkt, int seg, size_t len);
> +
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/platform/linux-generic/source/odp_buffer.c b/platform/linux-generic/source/odp_buffer.c
> index afbe96a..0169eec 100644
> --- a/platform/linux-generic/source/odp_buffer.c
> +++ b/platform/linux-generic/source/odp_buffer.c
> @@ -36,17 +36,6 @@ int odp_buffer_type(odp_buffer_t buf)
>   }
>   
>   
> -int odp_buffer_is_scatter(odp_buffer_t buf)
> -{
> -	odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf);
> -
> -	if (hdr->scatter.num_bufs == 0)
> -		return 0;
> -	else
> -		return 1;
> -}
> -
> -
>   int odp_buffer_is_valid(odp_buffer_t buf)
>   {
>   	odp_buffer_bits_t handle;
> diff --git a/platform/linux-generic/source/odp_buffer_pool.c b/platform/linux-generic/source/odp_buffer_pool.c
> index 25c9565..b73cbb2 100644
> --- a/platform/linux-generic/source/odp_buffer_pool.c
> +++ b/platform/linux-generic/source/odp_buffer_pool.c
> @@ -491,6 +491,15 @@ void odp_buffer_free(odp_buffer_t buf)
>   }
>   
>   
> +odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf)
> +{
> +	odp_buffer_hdr_t *hdr;
> +
> +	hdr = odp_buf_to_hdr(buf);
> +	return hdr->pool;
> +}
> +
> +
>   void odp_buffer_pool_print(odp_buffer_pool_t pool_id)
>   {
>   	pool_entry_t *pool;
> diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c
> index 99fcc6d..13e2471 100644
> --- a/platform/linux-generic/source/odp_packet.c
> +++ b/platform/linux-generic/source/odp_packet.c
> @@ -127,6 +127,26 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, size_t offset)
>   	odp_packet_hdr(pkt)->l4_offset = offset;
>   }
>   
> +
> +int odp_packet_is_segmented(odp_packet_t pkt)
> +{
> +	odp_buffer_hdr_t *buf_hdr = odp_buf_to_hdr((odp_buffer_t)pkt);
> +
> +	if (buf_hdr->scatter.num_bufs == 0)
> +		return 0;
> +	else
> +		return 1;
> +}
> +
> +
> +int odp_packet_seg_count(odp_packet_t pkt)
> +{
> +	odp_buffer_hdr_t *buf_hdr = odp_buf_to_hdr((odp_buffer_t)pkt);
> +
> +	return (int)buf_hdr->scatter.num_bufs + 1;
> +}
> +
> +
>   /**
>    * Simple packet parser: eth, VLAN, IP, TCP/UDP/ICMP
>    *

On 07/01/2014 11:08 AM, Petri Savolainen wrote:
> Started buffer/packet/SG-list API harmonizartion. New API terminology
> is introduced first in segments (SG list) API and buffer/packet APIs
> are changed to use same terms in following patches. Segments are
> introduced for packet type buffers only at this phase.
> 
> Term summary:
> - xxx_addr     = segment start address
> - xxx_size     = segment size
> - xxx_data     = data pointer (data start inside the segment)
> - xxx_data_len = data length
> 
> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> ---
>   include/helper/odp_packet_helper.h              |  14 ---
>   include/odp_buffer.h                            |  11 --
>   include/odp_buffer_pool.h                       |   9 +-
>   include/odp_packet.h                            | 148 ++++++++++++++++++++++++
>   platform/linux-generic/source/odp_buffer.c      |  11 --
>   platform/linux-generic/source/odp_buffer_pool.c |   9 ++
>   platform/linux-generic/source/odp_packet.c      |  20 ++++
>   7 files changed, 185 insertions(+), 37 deletions(-)
> 
> diff --git a/include/helper/odp_packet_helper.h b/include/helper/odp_packet_helper.h
> index eed866e..db12028 100644
> --- a/include/helper/odp_packet_helper.h
> +++ b/include/helper/odp_packet_helper.h
> @@ -89,20 +89,6 @@ static inline size_t odp_packet_buf_size(odp_packet_t pkt)
>   	return odp_buffer_size(buf);
>   }
>   
> -/**
> - * Helper: Tests if packet is part of a scatter/gather list
> - *
> - * @param pkt  Packet handle
> - *
> - * @return 1 if belongs to a scatter list, otherwise 0
> - */
> -static inline int odp_packet_is_scatter(odp_packet_t pkt)
> -{
> -	odp_buffer_t buf = odp_buffer_from_packet(pkt);
> -
> -	return odp_buffer_is_scatter(buf);
> -}
> -
>   
>   #ifdef __cplusplus
>   }
> diff --git a/include/odp_buffer.h b/include/odp_buffer.h
> index b3d6f4a..d8577fd 100644
> --- a/include/odp_buffer.h
> +++ b/include/odp_buffer.h
> @@ -19,13 +19,10 @@ extern "C" {
>   #endif
>   
>   
> -
>   #include <odp_std_types.h>
>   
>   
>   
> -
> -
>   /**
>    * ODP buffer
>    */
> @@ -68,14 +65,6 @@ int odp_buffer_type(odp_buffer_t buf);
>   #define ODP_BUFFER_TYPE_PACKET    2  /**< Packet buffer */
>   #define ODP_BUFFER_TYPE_TIMEOUT   3  /**< Timeout buffer */
>   
> -/**
> - * Tests if buffer is part of a scatter/gather list
> - *
> - * @param buf      Buffer handle
> - *
> - * @return 1 if belongs to a scatter list, otherwise 0
> - */
> -int odp_buffer_is_scatter(odp_buffer_t buf);
>   
>   /**
>    * Tests if buffer is valid
> diff --git a/include/odp_buffer_pool.h b/include/odp_buffer_pool.h
> index 9112489..69155cc 100644
> --- a/include/odp_buffer_pool.h
> +++ b/include/odp_buffer_pool.h
> @@ -70,7 +70,6 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char *name);
>   void odp_buffer_pool_print(odp_buffer_pool_t pool);
>   
>   
> -
>   /**
>    * Buffer alloc
>    *
> @@ -90,6 +89,14 @@ odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t pool);
>   void odp_buffer_free(odp_buffer_t buf);
>   
>   
> +/**
> + * Buffer pool of the buffer
> + *
> + * @param buf       Buffer handle
> + *
> + * @return Buffer pool the buffer was allocated from
> + */
> +odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
>   
>   
>   #ifdef __cplusplus
> diff --git a/include/odp_packet.h b/include/odp_packet.h
> index 59759bb..8ae6410 100644
> --- a/include/odp_packet.h
> +++ b/include/odp_packet.h
> @@ -220,6 +220,154 @@ void odp_packet_print(odp_packet_t pkt);
>    */
>   int odp_packet_copy(odp_packet_t pkt_dst, odp_packet_t pkt_src);
>   
> +/**
> + * Tests if packet is segmented (a scatter/gather list)
> + *
> + * @param pkt  Packet handle
> + *
> + * @return Non-zero if packet is segmented, otherwise 0
> + */
> +int odp_packet_is_segmented(odp_packet_t pkt);
> +
> +/**
> + * Segment count
> + *
> + * Returns number of segments in the packet. A packet has always at least one
> + * segment (the packet buffer itself).
> + *
> + * @param pkt  Packet handle
> + *
> + * @return Segment count
> + */
> +int odp_packet_seg_count(odp_packet_t pkt);
> +
> +/**
> + * Segment start address
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment start address
> + */
> +void *odp_packet_seg_addr(odp_packet_t pkt, int seg);

Most of APIs in this file accepts segment index as a parameter.
That makes sense in case segments often accessed randomly.
But IMO the most common case is an iteration over segments sequentially.
Current API is not efficient for such use case.
For example if one needs to inspect data in all segments he will do
something like this:

int seg_count = odp_packet_seg_count(pkt);
for (int seg = 0; seg < seg_count; seg++) {
	char *data = odp_packet_seg_data(pkt, seg);
	size_t len = odp_packet_seg_data_len(pkt, seg);
	/* Do something with data */
}

If segments are handled as linked lists by implementation, this will
cause 2*(N^2) list hops, where N is seg_count.

Maybe better to introduce a new handle type odp_pkt_segment_t
and add iterator function:

/**
 * Get next packet segment
 *
 * @param pkt  Packet handle
 * @param seg  Current segment handle.
 *
 * @return First segment if seg == ODP_PKT_SEGMENT_INVALID.
 *         ODP_PKT_SEGMENT_INVALID if seg is the last segment.
 *         Next packet segment otherwise.
 */
odp_pkt_segment_t odp_packet_next_segment(odp_packet_t pkt, odp_pkt_segment_t seg);
....
size_t odp_packet_seg_size(odp_packet_t pkt, odp_pkt_segment_t seg);
void *odp_packet_seg_data(odp_packet_t pkt, odp_pkt_segment_t seg);
....


So iteration over segments can look like:

odp_pkt_segment_t seg = odp_packet_next_segment(pkt, ODP_PKT_SEGMENT_INVALID);
while (seg != ODP_PKT_SEGMENT_INVALID) {
	char *data = odp_packet_seg_data(pkt, seg);
	size_t len = odp_packet_seg_data_len(pkt, seg);
	/* Do something with data */
	seg = odp_packet_next_segment(pkt, seg);
}

In this case implementation based on linked lists can do only N hops.

> +
> +/**
> + * Segment maximum data size
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment maximum data size
> + */
> +size_t odp_packet_seg_size(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment data address
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment data address
> + */
> +void *odp_packet_seg_data(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment data length
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Segment data length
> + */
> +size_t odp_packet_seg_data_len(odp_packet_t pkt, int seg);
> +
> +/**
> + * Segment headroom
> + *
> + * seg_headroom = seg_data - seg_addr
> + *
> + * @param pkt  Packet handle
> + * @param seg  Segment index (0 ... seg_count-1)
> + *
> + * @return Number of octets from seg_addr to seg_data
> + */
> +size_t odp_packet_seg_headroom(odp_packet_t pkt, int seg);

My understanding was that headroom/tailroom is an feature of a packet,
but not segment. What is the value of having it at segment level?

On 07/02/2014 12:17 PM, Maxim Uvarov wrote:
> Applied, thanks!

I assume this patch had to be called RFC.

Good point and is also why you probably want a single call to retrieve
segment addr and length rather than two separate calls.

I'm still a bit concerned about SW-visible/controlled packet segmentation
as this is precisely the sort of thing that HW does far more efficiently
than SW.  An application deals with logical packets that may be physically
represented as a chain of buffers.  From an application standpoint the key
notions are an application-visible address that it can use to access a
"window" on the packet, the size of that window, and the byte offset of
that window from the start of the packet.

So to process an entire packet an application would use something like:

int seg_size  = odp_pkt_seg_size(pkt);
int offset       = 0;
int pkt_len     = odp_pkt_len(pkt);

while (offset < pkt_len) {
    odp_pkt_seg_t *addr = odp_pkt_seg(pkt,offset);
    ...do something with this segment
    offset += seg_size;
}

This preserves implementation flexibility since seg_size is
implementation-specific.  In most cases a data plane application is only
interested in offset 0 since the first segment is where headers are
located.

Bill



On Wed, Jul 2, 2014 at 5:47 AM, Taras Kondratiuk <
taras.kondratiuk@linaro.org> wrote:

> On 07/02/2014 12:17 PM, Maxim Uvarov wrote:
>
>> Applied, thanks!
>>
>
> I assume this patch had to be called RFC.
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>

On 07/02/2014 02:47 PM, Taras Kondratiuk wrote:
> On 07/02/2014 12:17 PM, Maxim Uvarov wrote:
>> Applied, thanks!
>
> I assume this patch had to be called RFC.
>
This patch is on top now. If you think that it's to early to include it 
please let me know. I can drop it from git.

Maxim.


> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp

Hi Bill,

The initial proposal has index number for accessing the segments
void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);

The idea of returning size per individual segments was suggested as the
size of the last segment will usually be smaller than the size of the
remaining segments. Also the size of the valid data inside each segment
could be varied by adding valid data in the headroom.

Regards,
Bala


On 2 July 2014 16:53, Bill Fischofer <bill.fischofer@linaro.org> wrote:

> Good point and is also why you probably want a single call to retrieve
> segment addr and length rather than two separate calls.
>
> I'm still a bit concerned about SW-visible/controlled packet segmentation
> as this is precisely the sort of thing that HW does far more efficiently
> than SW.  An application deals with logical packets that may be physically
> represented as a chain of buffers.  From an application standpoint the key
> notions are an application-visible address that it can use to access a
> "window" on the packet, the size of that window, and the byte offset of
> that window from the start of the packet.
>
> So to process an entire packet an application would use something like:
>
> int seg_size  = odp_pkt_seg_size(pkt);
> int offset       = 0;
> int pkt_len     = odp_pkt_len(pkt);
>
> while (offset < pkt_len) {
>     odp_pkt_seg_t *addr = odp_pkt_seg(pkt,offset);
>     ...do something with this segment
>     offset += seg_size;
> }
>
> This preserves implementation flexibility since seg_size is
> implementation-specific.  In most cases a data plane application is only
> interested in offset 0 since the first segment is where headers are
> located.
>
> Bill
>
>
>
> On Wed, Jul 2, 2014 at 5:47 AM, Taras Kondratiuk <
> taras.kondratiuk@linaro.org> wrote:
>
>> On 07/02/2014 12:17 PM, Maxim Uvarov wrote:
>>
>>> Applied, thanks!
>>>
>>
>> I assume this patch had to be called RFC.
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>

Based on recent discussions, I'd change the example to:

int offset   = 0;
int pkt_len = odp_pkt_len(pkt);
int seg_size;

while (offset < pkt_len) {
    odp_pkt_seg_t *addr = odp_pkt_seg(pkt,offset,*seg_size);
    ...do something with this segment
    offset += seg_size;
}




On Wed, Jul 2, 2014 at 6:52 AM, Bala Manoharan <bala.manoharan@linaro.org>
wrote:

> Hi Bill,
>
> The initial proposal has index number for accessing the segments
> void* odp_buffer_segment_addr(odp_buffer_t buf, int n, size_t* size);
>
> The idea of returning size per individual segments was suggested as the
> size of the last segment will usually be smaller than the size of the
> remaining segments. Also the size of the valid data inside each segment
> could be varied by adding valid data in the headroom.
>
> Regards,
> Bala
>
>
> On 2 July 2014 16:53, Bill Fischofer <bill.fischofer@linaro.org> wrote:
>
>> Good point and is also why you probably want a single call to retrieve
>> segment addr and length rather than two separate calls.
>>
>> I'm still a bit concerned about SW-visible/controlled packet segmentation
>> as this is precisely the sort of thing that HW does far more efficiently
>> than SW.  An application deals with logical packets that may be physically
>> represented as a chain of buffers.  From an application standpoint the key
>> notions are an application-visible address that it can use to access a
>> "window" on the packet, the size of that window, and the byte offset of
>> that window from the start of the packet.
>>
>> So to process an entire packet an application would use something like:
>>
>> int seg_size  = odp_pkt_seg_size(pkt);
>> int offset       = 0;
>> int pkt_len     = odp_pkt_len(pkt);
>>
>> while (offset < pkt_len) {
>>     odp_pkt_seg_t *addr = odp_pkt_seg(pkt,offset);
>>     ...do something with this segment
>>     offset += seg_size;
>> }
>>
>> This preserves implementation flexibility since seg_size is
>> implementation-specific.  In most cases a data plane application is only
>> interested in offset 0 since the first segment is where headers are
>> located.
>>
>> Bill
>>
>>
>>
>> On Wed, Jul 2, 2014 at 5:47 AM, Taras Kondratiuk <
>> taras.kondratiuk@linaro.org> wrote:
>>
>>> On 07/02/2014 12:17 PM, Maxim Uvarov wrote:
>>>
>>>> Applied, thanks!
>>>>
>>>
>>> I assume this patch had to be called RFC.
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>

> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Taras Kondratiuk
> Sent: Wednesday, July 02, 2014 1:43 PM
> To: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH] Packet segment API started
> 
> On 07/01/2014 11:08 AM, Petri Savolainen wrote:
> > Started buffer/packet/SG-list API harmonizartion. New API terminology
> > is introduced first in segments (SG list) API and buffer/packet APIs
> > are changed to use same terms in following patches. Segments are
> > introduced for packet type buffers only at this phase.
> >
> > Term summary:
> > - xxx_addr     = segment start address
> > - xxx_size     = segment size
> > - xxx_data     = data pointer (data start inside the segment)
> > - xxx_data_len = data length
> >
> > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> > ---
> >   include/helper/odp_packet_helper.h              |  14 ---
> >   include/odp_buffer.h                            |  11 --
> >   include/odp_buffer_pool.h                       |   9 +-
> >   include/odp_packet.h                            | 148
> ++++++++++++++++++++++++
> >   platform/linux-generic/source/odp_buffer.c      |  11 --
> >   platform/linux-generic/source/odp_buffer_pool.c |   9 ++
> >   platform/linux-generic/source/odp_packet.c      |  20 ++++
> >   7 files changed, 185 insertions(+), 37 deletions(-)
> >
> > diff --git a/include/helper/odp_packet_helper.h
> b/include/helper/odp_packet_helper.h
> > index eed866e..db12028 100644
> > --- a/include/helper/odp_packet_helper.h
> > +++ b/include/helper/odp_packet_helper.h
> > @@ -89,20 +89,6 @@ static inline size_t
> odp_packet_buf_size(odp_packet_t pkt)
> >   	return odp_buffer_size(buf);
> >   }
> >
> > -/**
> > - * Helper: Tests if packet is part of a scatter/gather list
> > - *
> > - * @param pkt  Packet handle
> > - *
> > - * @return 1 if belongs to a scatter list, otherwise 0
> > - */
> > -static inline int odp_packet_is_scatter(odp_packet_t pkt)
> > -{
> > -	odp_buffer_t buf = odp_buffer_from_packet(pkt);
> > -
> > -	return odp_buffer_is_scatter(buf);
> > -}
> > -
> >
> >   #ifdef __cplusplus
> >   }
> > diff --git a/include/odp_buffer.h b/include/odp_buffer.h
> > index b3d6f4a..d8577fd 100644
> > --- a/include/odp_buffer.h
> > +++ b/include/odp_buffer.h
> > @@ -19,13 +19,10 @@ extern "C" {
> >   #endif
> >
> >
> > -
> >   #include <odp_std_types.h>
> >
> >
> >
> > -
> > -
> >   /**
> >    * ODP buffer
> >    */
> > @@ -68,14 +65,6 @@ int odp_buffer_type(odp_buffer_t buf);
> >   #define ODP_BUFFER_TYPE_PACKET    2  /**< Packet buffer */
> >   #define ODP_BUFFER_TYPE_TIMEOUT   3  /**< Timeout buffer */
> >
> > -/**
> > - * Tests if buffer is part of a scatter/gather list
> > - *
> > - * @param buf      Buffer handle
> > - *
> > - * @return 1 if belongs to a scatter list, otherwise 0
> > - */
> > -int odp_buffer_is_scatter(odp_buffer_t buf);
> >
> >   /**
> >    * Tests if buffer is valid
> > diff --git a/include/odp_buffer_pool.h b/include/odp_buffer_pool.h
> > index 9112489..69155cc 100644
> > --- a/include/odp_buffer_pool.h
> > +++ b/include/odp_buffer_pool.h
> > @@ -70,7 +70,6 @@ odp_buffer_pool_t odp_buffer_pool_lookup(const char
> *name);
> >   void odp_buffer_pool_print(odp_buffer_pool_t pool);
> >
> >
> > -
> >   /**
> >    * Buffer alloc
> >    *
> > @@ -90,6 +89,14 @@ odp_buffer_t odp_buffer_alloc(odp_buffer_pool_t
> pool);
> >   void odp_buffer_free(odp_buffer_t buf);
> >
> >
> > +/**
> > + * Buffer pool of the buffer
> > + *
> > + * @param buf       Buffer handle
> > + *
> > + * @return Buffer pool the buffer was allocated from
> > + */
> > +odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
> >
> >
> >   #ifdef __cplusplus
> > diff --git a/include/odp_packet.h b/include/odp_packet.h
> > index 59759bb..8ae6410 100644
> > --- a/include/odp_packet.h
> > +++ b/include/odp_packet.h
> > @@ -220,6 +220,154 @@ void odp_packet_print(odp_packet_t pkt);
> >    */
> >   int odp_packet_copy(odp_packet_t pkt_dst, odp_packet_t pkt_src);
> >
> > +/**
> > + * Tests if packet is segmented (a scatter/gather list)
> > + *
> > + * @param pkt  Packet handle
> > + *
> > + * @return Non-zero if packet is segmented, otherwise 0
> > + */
> > +int odp_packet_is_segmented(odp_packet_t pkt);
> > +
> > +/**
> > + * Segment count
> > + *
> > + * Returns number of segments in the packet. A packet has always at
> least one
> > + * segment (the packet buffer itself).
> > + *
> > + * @param pkt  Packet handle
> > + *
> > + * @return Segment count
> > + */
> > +int odp_packet_seg_count(odp_packet_t pkt);
> > +
> > +/**
> > + * Segment start address
> > + *
> > + * @param pkt  Packet handle
> > + * @param seg  Segment index (0 ... seg_count-1)
> > + *
> > + * @return Segment start address
> > + */
> > +void *odp_packet_seg_addr(odp_packet_t pkt, int seg);
> 
> Most of APIs in this file accepts segment index as a parameter.
> That makes sense in case segments often accessed randomly.
> But IMO the most common case is an iteration over segments sequentially.
> Current API is not efficient for such use case.
> For example if one needs to inspect data in all segments he will do
> something like this:
> 
> int seg_count = odp_packet_seg_count(pkt);
> for (int seg = 0; seg < seg_count; seg++) {
> 	char *data = odp_packet_seg_data(pkt, seg);
> 	size_t len = odp_packet_seg_data_len(pkt, seg);
> 	/* Do something with data */
> }
> 
> If segments are handled as linked lists by implementation, this will
> cause 2*(N^2) list hops, where N is seg_count.
> 
> Maybe better to introduce a new handle type odp_pkt_segment_t
> and add iterator function:
> 
> /**
>  * Get next packet segment
>  *
>  * @param pkt  Packet handle
>  * @param seg  Current segment handle.
>  *
>  * @return First segment if seg == ODP_PKT_SEGMENT_INVALID.
>  *         ODP_PKT_SEGMENT_INVALID if seg is the last segment.
>  *         Next packet segment otherwise.
>  */
> odp_pkt_segment_t odp_packet_next_segment(odp_packet_t pkt,
> odp_pkt_segment_t seg);
> ....
> size_t odp_packet_seg_size(odp_packet_t pkt, odp_pkt_segment_t seg);
> void *odp_packet_seg_data(odp_packet_t pkt, odp_pkt_segment_t seg);
> ....
> 
> 
> So iteration over segments can look like:
> 
> odp_pkt_segment_t seg = odp_packet_next_segment(pkt,
> ODP_PKT_SEGMENT_INVALID);
> while (seg != ODP_PKT_SEGMENT_INVALID) {
> 	char *data = odp_packet_seg_data(pkt, seg);
> 	size_t len = odp_packet_seg_data_len(pkt, seg);
> 	/* Do something with data */
> 	seg = odp_packet_next_segment(pkt, seg);
> }
> 
> In this case implementation based on linked lists can do only N hops.

I'll look into this. I avoided introducing segment as a handle, but may it works OK if segment handle is never referenced alone (always with packet handle). That way both table and linked list implementations are OK.



> 
> > +
> > +/**
> > + * Segment maximum data size
> > + *
> > + * @param pkt  Packet handle
> > + * @param seg  Segment index (0 ... seg_count-1)
> > + *
> > + * @return Segment maximum data size
> > + */
> > +size_t odp_packet_seg_size(odp_packet_t pkt, int seg);
> > +
> > +/**
> > + * Segment data address
> > + *
> > + * @param pkt  Packet handle
> > + * @param seg  Segment index (0 ... seg_count-1)
> > + *
> > + * @return Segment data address
> > + */
> > +void *odp_packet_seg_data(odp_packet_t pkt, int seg);
> > +
> > +/**
> > + * Segment data length
> > + *
> > + * @param pkt  Packet handle
> > + * @param seg  Segment index (0 ... seg_count-1)
> > + *
> > + * @return Segment data length
> > + */
> > +size_t odp_packet_seg_data_len(odp_packet_t pkt, int seg);
> > +
> > +/**
> > + * Segment headroom
> > + *
> > + * seg_headroom = seg_data - seg_addr
> > + *
> > + * @param pkt  Packet handle
> > + * @param seg  Segment index (0 ... seg_count-1)
> > + *
> > + * @return Number of octets from seg_addr to seg_data
> > + */
> > +size_t odp_packet_seg_headroom(odp_packet_t pkt, int seg);
> 
> My understanding was that headroom/tailroom is an feature of a packet,
> but not segment. What is the value of having it at segment level?

These return the current head/tailrooms per segment. User can request e.g. packet input to leave N bytes headroom for the first segment, or all segments. If HW supports only headroom for first segment, then seg_headroom(pkt, 0) returns N and all others return zero.

-Petri