diff mbox

Adding odp_packet_copy

Message ID 1396027404-34935-1-git-send-email-ciprian.barbu@linaro.org
State Accepted
Commit 248c00d3cd3855955955e75f2cc4ef18ef596fb9
Headers show

Commit Message

Ciprian Barbu March 28, 2014, 5:23 p.m. UTC
Resending due to missing [lng-odp]

odp_packet_copy for copying pkt contents and metadata

Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org>
---
 include/odp_packet.h                          |  9 +++++++++
 platform/linux-generic/source/odp_packet.c    | 24 ++++++++++++++++++++++++
 test/packet_netmap/odp_example_pktio_netmap.c | 11 +----------
 3 files changed, 34 insertions(+), 10 deletions(-)

Comments

Petri Savolainen March 31, 2014, 8:31 a.m. UTC | #1
Hi,


On Friday, 28 March 2014 19:23:24 UTC+2, Ciprian Barbu wrote:

> Resending due to missing [lng-odp] 
>
> odp_packet_copy for copying pkt contents and metadata 
>
> Signed-off-by: Ciprian Barbu <cipria...@linaro.org <javascript:>> 
> --- 
>  include/odp_packet.h                          |  9 +++++++++ 
>  platform/linux-generic/source/odp_packet.c    | 24 
> ++++++++++++++++++++++++ 
>  test/packet_netmap/odp_example_pktio_netmap.c | 11 +---------- 
>  3 files changed, 34 insertions(+), 10 deletions(-) 
>
> diff --git a/include/odp_packet.h b/include/odp_packet.h 
> index 3c7c9d0..c0bedcd 100644 
> --- a/include/odp_packet.h 
> +++ b/include/odp_packet.h 
> @@ -209,6 +209,15 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, 
> size_t offset); 
>   */ 
>  void odp_packet_print(odp_packet_t pkt); 
>   
> +/** 
> + * Copy contents and metadata from pkt_src to pkt_dst 
> + * Useful when creating copies of packets 
> + * 
> + * @param pkt_src Source packet 
> + * @param pkt_dst Destination packet 
> + */ 
> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst); 
>

void odp_packet_copy(odp_packet_t dst, odp_packet_t src);
It should be other way around: dst on left, src on rigth (compared with 
memcpy() or x = y;). Could also 
return an error if one of the packets are broken and cannot copy.

 

> + 
>  #ifdef __cplusplus 
>  } 
>  #endif 
> diff --git a/platform/linux-generic/source/odp_packet.c 
> b/platform/linux-generic/source/odp_packet.c 
> index b990222..47cabd3 100644 
> --- a/platform/linux-generic/source/odp_packet.c 
> +++ b/platform/linux-generic/source/odp_packet.c 
> @@ -331,3 +331,27 @@ void odp_packet_print(odp_packet_t pkt) 
>          printf("\n%s\n", str); 
>  } 
>   
> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst) 
> +{ 
> +        odp_packet_hdr_t *const pkt_hdr_src = odp_packet_hdr(pkt_src); 
> +        odp_packet_hdr_t *const pkt_hdr_dst = odp_packet_hdr(pkt_dst); 
> +        const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, 
> buf_hdr); 
>

start_offset = 0 ?? Anyway, it should copy the whole packet, including the 
buffer header (packet is a buffer).
 

> +        odp_pktio_t input; 
> +        uint8_t *start_src; 
> +        uint8_t *start_dst; 
> +        size_t len; 
> + 
> +        /* Preserve input */ 
> +        input = pkt_hdr_dst->input;


Why this field is not copied over? Dst packet should be exact copy of the 
src packet.

-Petri 

>
> + 
> +        start_src = (uint8_t *)pkt_hdr_src + start_offset; 
> +        start_dst = (uint8_t *)pkt_hdr_dst + start_offset; 
> + 
> +        len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset; 
> +        len += pkt_hdr_src->frame_offset + pkt_hdr_src->frame_len; 
> + 
> +        memcpy(start_dst, start_src, len); 
> + 
> +        /* Restore input */ 
> +        pkt_hdr_dst->input = input; 
> +} 
> diff --git a/test/packet_netmap/odp_example_pktio_netmap.c 
> b/test/packet_netmap/odp_example_pktio_netmap.c 
> index 2d74f8a..f9ed3b3 100644 
> --- a/test/packet_netmap/odp_example_pktio_netmap.c 
> +++ b/test/packet_netmap/odp_example_pktio_netmap.c 
> @@ -155,20 +155,11 @@ static void *pktio_queue_thread(void *arg) 
>                  if (args->thread[pktio_nr].netmap_mode == 
> ODP_NETMAP_MODE_HW) { 
>                          odp_packet_t pkt_copy; 
>                          odp_buffer_t buf_copy; 
> -                        size_t frame_len = odp_packet_get_len(pkt); 
> -                        size_t l2_offset = odp_packet_l2_offset(pkt); 
> -                        size_t l3_offset = odp_packet_l3_offset(pkt); 
>   
>                          buf_copy = odp_buffer_alloc(pkt_pool); 
>                          pkt_copy = odp_packet_from_buffer(buf_copy); 
>   
> -                        odp_packet_init(pkt_copy); 
> -                        odp_packet_set_len(pkt_copy, frame_len); 
> -                        odp_packet_set_l2_offset(pkt_copy, l2_offset); 
> -                        odp_packet_set_l3_offset(pkt_copy, l3_offset); 
> - 
> -                        memcpy(odp_buffer_addr(pkt_copy), 
> -                               odp_buffer_addr(pkt), frame_len); 
> +                        odp_packet_copy(pkt, pkt_copy); 
>   
>                          swap_pkt_addrs(&pkt_copy, 1); 
>   
> -- 
> 1.8.3.2 
>
>
Ciprian Barbu March 31, 2014, 9:28 a.m. UTC | #2
On Mon, Mar 31, 2014 at 11:31 AM, Petri Savolainen <
petri.savolainen@linaro.org> wrote:

> Hi,
>
>
>
> On Friday, 28 March 2014 19:23:24 UTC+2, Ciprian Barbu wrote:
>
>> Resending due to missing [lng-odp]
>>
>> odp_packet_copy for copying pkt contents and metadata
>>
>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org>
>> ---
>>  include/odp_packet.h                          |  9 +++++++++
>>  platform/linux-generic/source/odp_packet.c    | 24
>> ++++++++++++++++++++++++
>>  test/packet_netmap/odp_example_pktio_netmap.c | 11 +----------
>>  3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/odp_packet.h b/include/odp_packet.h
>> index 3c7c9d0..c0bedcd 100644
>> --- a/include/odp_packet.h
>> +++ b/include/odp_packet.h
>> @@ -209,6 +209,15 @@ void odp_packet_set_l4_offset(odp_packet_t pkt,
>> size_t offset);
>>   */
>>  void odp_packet_print(odp_packet_t pkt);
>>
>> +/**
>> + * Copy contents and metadata from pkt_src to pkt_dst
>> + * Useful when creating copies of packets
>> + *
>> + * @param pkt_src Source packet
>> + * @param pkt_dst Destination packet
>> + */
>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst);
>>
>
> void odp_packet_copy(odp_packet_t dst, odp_packet_t src);
> It should be other way around: dst on left, src on rigth (compared with
> memcpy() or x = y;). Could also
> return an error if one of the packets are broken and cannot copy.
>

Ok, I will switch the parameters around. About broken packages, what should
I check there, the error flags?

>
>
>
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif
>> diff --git a/platform/linux-generic/source/odp_packet.c
>> b/platform/linux-generic/source/odp_packet.c
>> index b990222..47cabd3 100644
>> --- a/platform/linux-generic/source/odp_packet.c
>> +++ b/platform/linux-generic/source/odp_packet.c
>> @@ -331,3 +331,27 @@ void odp_packet_print(odp_packet_t pkt)
>>          printf("\n%s\n", str);
>>  }
>>
>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst)
>> +{
>> +        odp_packet_hdr_t *const pkt_hdr_src = odp_packet_hdr(pkt_src);
>> +        odp_packet_hdr_t *const pkt_hdr_dst = odp_packet_hdr(pkt_dst);
>> +        const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t,
>> buf_hdr);
>>
>
> start_offset = 0 ?? Anyway, it should copy the whole packet, including the
> buffer header (packet is a buffer).
>

the buffer header  contains the buffer index in the pool and the
originating pool, I suppose those should not be modified. But the start
offset is wrong, it should skip the buf header in this case.

>
>
>> +        odp_pktio_t input;
>> +        uint8_t *start_src;
>> +        uint8_t *start_dst;
>> +        size_t len;
>> +
>> +        /* Preserve input */
>> +        input = pkt_hdr_dst->input;
>
>
> Why this field is not copied over? Dst packet should be exact copy of the
> src packet.
>

I thought the pktio should not be touched. I don't need it in my code so I
will give up on it.

>
>
> -Petri
>
>>
>> +
>> +        start_src = (uint8_t *)pkt_hdr_src + start_offset;
>> +        start_dst = (uint8_t *)pkt_hdr_dst + start_offset;
>> +
>> +        len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset;
>> +        len += pkt_hdr_src->frame_offset + pkt_hdr_src->frame_len;
>> +
>> +        memcpy(start_dst, start_src, len);
>> +
>> +        /* Restore input */
>> +        pkt_hdr_dst->input = input;
>> +}
>> diff --git a/test/packet_netmap/odp_example_pktio_netmap.c
>> b/test/packet_netmap/odp_example_pktio_netmap.c
>> index 2d74f8a..f9ed3b3 100644
>> --- a/test/packet_netmap/odp_example_pktio_netmap.c
>> +++ b/test/packet_netmap/odp_example_pktio_netmap.c
>> @@ -155,20 +155,11 @@ static void *pktio_queue_thread(void *arg)
>>                  if (args->thread[pktio_nr].netmap_mode ==
>> ODP_NETMAP_MODE_HW) {
>>                          odp_packet_t pkt_copy;
>>                          odp_buffer_t buf_copy;
>> -                        size_t frame_len = odp_packet_get_len(pkt);
>> -                        size_t l2_offset = odp_packet_l2_offset(pkt);
>> -                        size_t l3_offset = odp_packet_l3_offset(pkt);
>>
>>                          buf_copy = odp_buffer_alloc(pkt_pool);
>>                          pkt_copy = odp_packet_from_buffer(buf_copy);
>>
>> -                        odp_packet_init(pkt_copy);
>> -                        odp_packet_set_len(pkt_copy, frame_len);
>> -                        odp_packet_set_l2_offset(pkt_copy, l2_offset);
>> -                        odp_packet_set_l3_offset(pkt_copy, l3_offset);
>> -
>> -                        memcpy(odp_buffer_addr(pkt_copy),
>> -                               odp_buffer_addr(pkt), frame_len);
>> +                        odp_packet_copy(pkt, pkt_copy);
>>
>>                          swap_pkt_addrs(&pkt_copy, 1);
>>
>> --
>> 1.8.3.2
>>
>>
/Ciprian
Petri Savolainen March 31, 2014, 11:39 a.m. UTC | #3
On Monday, 31 March 2014 12:28:28 UTC+3, Ciprian Barbu wrote:

>
>
>
> On Mon, Mar 31, 2014 at 11:31 AM, Petri Savolainen <petri.sa...@linaro.org<javascript:>
> > wrote:
>
>> Hi,
>>
>>
>>
>> On Friday, 28 March 2014 19:23:24 UTC+2, Ciprian Barbu wrote:
>>
>>> Resending due to missing [lng-odp] 
>>>
>>> odp_packet_copy for copying pkt contents and metadata 
>>>
>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org> 
>>> --- 
>>>  include/odp_packet.h                          |  9 +++++++++ 
>>>  platform/linux-generic/source/odp_packet.c    | 24 
>>> ++++++++++++++++++++++++ 
>>>  test/packet_netmap/odp_example_pktio_netmap.c | 11 +---------- 
>>>  3 files changed, 34 insertions(+), 10 deletions(-) 
>>>
>>> diff --git a/include/odp_packet.h b/include/odp_packet.h 
>>> index 3c7c9d0..c0bedcd 100644 
>>> --- a/include/odp_packet.h 
>>> +++ b/include/odp_packet.h 
>>> @@ -209,6 +209,15 @@ void odp_packet_set_l4_offset(odp_packet_t pkt, 
>>> size_t offset); 
>>>   */ 
>>>  void odp_packet_print(odp_packet_t pkt); 
>>>   
>>> +/** 
>>> + * Copy contents and metadata from pkt_src to pkt_dst 
>>> + * Useful when creating copies of packets 
>>> + * 
>>> + * @param pkt_src Source packet 
>>> + * @param pkt_dst Destination packet 
>>> + */ 
>>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst); 
>>>
>>
>> void odp_packet_copy(odp_packet_t dst, odp_packet_t src);
>> It should be other way around: dst on left, src on rigth (compared with 
>> memcpy() or x = y;). Could also 
>> return an error if one of the packets are broken and cannot copy.
>>
>
> Ok, I will switch the parameters around. About broken packages, what 
> should I check there, the error flags?
>

Invalid packets or dst packet smaller than src packet, ...

 

>  
>
>>
>>  
>>
>>> + 
>>>  #ifdef __cplusplus 
>>>  } 
>>>  #endif 
>>> diff --git a/platform/linux-generic/source/odp_packet.c 
>>> b/platform/linux-generic/source/odp_packet.c 
>>> index b990222..47cabd3 100644 
>>> --- a/platform/linux-generic/source/odp_packet.c 
>>> +++ b/platform/linux-generic/source/odp_packet.c 
>>> @@ -331,3 +331,27 @@ void odp_packet_print(odp_packet_t pkt) 
>>>          printf("\n%s\n", str); 
>>>  } 
>>>   
>>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst) 
>>> +{ 
>>> +        odp_packet_hdr_t *const pkt_hdr_src = odp_packet_hdr(pkt_src); 
>>> +        odp_packet_hdr_t *const pkt_hdr_dst = odp_packet_hdr(pkt_dst); 
>>> +        const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, 
>>> buf_hdr); 
>>>
>>
>> start_offset = 0 ?? Anyway, it should copy the whole packet, including 
>> the buffer header (packet is a buffer).
>>
>
> the buffer header  contains the buffer index in the pool and the 
> originating pool, I suppose those should not be modified. But the start 
> offset is wrong, it should skip the buf header in this case.
>

Sorry, read that ODP_FIELD_OFFSETOF. Why not use sizeof(odp_buffer_hdr_t). 
Right, all buffer header fields cannot be copied, but e.g. cur_offset 
should be copied and possible scatters.


 

>  
>>
>>> +        odp_pktio_t input; 
>>> +        uint8_t *start_src; 
>>> +        uint8_t *start_dst; 
>>> +        size_t len; 
>>> + 
>>> +        /* Preserve input */ 
>>> +        input = pkt_hdr_dst->input;
>>
>>
>> Why this field is not copied over? Dst packet should be exact copy of the 
>> src packet.
>>
>
> I thought the pktio should not be touched. I don't need it in my code so I 
> will give up on it.
>

input is metadata (the input interface id) and must be copied also (it's 
not odp_packet_copy_except_input_port() :) ).
 

> + 
>>> +        start_src = (uint8_t *)pkt_hdr_src + start_offset; 
>>> +        start_dst = (uint8_t *)pkt_hdr_dst + start_offset; 
>>> + 
>>> +        len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset; 
>>> +        len += pkt_hdr_src->frame_offset + pkt_hdr_src->frame_len; 
>>> + 
>>> +        memcpy(start_dst, start_src, len); 
>>> + 
>>> +        /* Restore input */ 
>>> +        pkt_hdr_dst->input = input; 
>>> +} 
>>> diff --git a/test/packet_netmap/odp_example_pktio_netmap.c 
>>> b/test/packet_netmap/odp_example_pktio_netmap.c 
>>> index 2d74f8a..f9ed3b3 100644 
>>> --- a/test/packet_netmap/odp_example_pktio_netmap.c 
>>> +++ b/test/packet_netmap/odp_example_pktio_netmap.c 
>>> @@ -155,20 +155,11 @@ static void *pktio_queue_thread(void *arg) 
>>>
>>

BTW, there synchronization error here (line before the patched line under):

buf = odp_schedule(NULL);
pkt = odp_packet_from_buffer(buf);
...
odp_queue_enq(args->thread[pktio_nr].bridge_q, buf);

You enqueued the buf already and with that lost the ownership to it (and to 
pkt). When you copy data from it the receiver may have already 
freed/overwritten the data.

-Petri

 

>                  if (args->thread[pktio_nr].netmap_mode == 
>>> ODP_NETMAP_MODE_HW) { 
>>>                          odp_packet_t pkt_copy; 
>>>                          odp_buffer_t buf_copy; 
>>> -                        size_t frame_len = odp_packet_get_len(pkt); 
>>> -                        size_t l2_offset = odp_packet_l2_offset(pkt); 
>>> -                        size_t l3_offset = odp_packet_l3_offset(pkt); 
>>>   
>>>                          buf_copy = odp_buffer_alloc(pkt_pool); 
>>>                          pkt_copy = odp_packet_from_buffer(buf_copy); 
>>>   
>>> -                        odp_packet_init(pkt_copy); 
>>> -                        odp_packet_set_len(pkt_copy, frame_len); 
>>> -                        odp_packet_set_l2_offset(pkt_copy, l2_offset); 
>>> -                        odp_packet_set_l3_offset(pkt_copy, l3_offset); 
>>> - 
>>> -                        memcpy(odp_buffer_addr(pkt_copy), 
>>> -                               odp_buffer_addr(pkt), frame_len); 
>>> +                        odp_packet_copy(pkt, pkt_copy); 
>>>   
>>>                          swap_pkt_addrs(&pkt_copy, 1); 
>>>   
>>> -- 
>>> 1.8.3.2 
>>>
>>>
> /Ciprian 
>
>
Ciprian Barbu April 1, 2014, 2:06 p.m. UTC | #4
On Mon, Mar 31, 2014 at 2:39 PM, Petri Savolainen <
petri.savolainen@linaro.org> wrote:

>
>
> On Monday, 31 March 2014 12:28:28 UTC+3, Ciprian Barbu wrote:
>
>>
>>
>>
>> On Mon, Mar 31, 2014 at 11:31 AM, Petri Savolainen <
>> petri.sa...@linaro.org> wrote:
>>
>>> Hi,
>>>
>>>
>>>
>>> On Friday, 28 March 2014 19:23:24 UTC+2, Ciprian Barbu wrote:
>>>
>>>> Resending due to missing [lng-odp]
>>>>
>>>> odp_packet_copy for copying pkt contents and metadata
>>>>
>>>> Signed-off-by: Ciprian Barbu <cipria...@linaro.org>
>>>> ---
>>>>  include/odp_packet.h                          |  9 +++++++++
>>>>  platform/linux-generic/source/odp_packet.c    | 24
>>>> ++++++++++++++++++++++++
>>>>  test/packet_netmap/odp_example_pktio_netmap.c | 11 +----------
>>>>  3 files changed, 34 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/include/odp_packet.h b/include/odp_packet.h
>>>> index 3c7c9d0..c0bedcd 100644
>>>> --- a/include/odp_packet.h
>>>> +++ b/include/odp_packet.h
>>>> @@ -209,6 +209,15 @@ void odp_packet_set_l4_offset(odp_packet_t pkt,
>>>> size_t offset);
>>>>   */
>>>>  void odp_packet_print(odp_packet_t pkt);
>>>>
>>>> +/**
>>>> + * Copy contents and metadata from pkt_src to pkt_dst
>>>> + * Useful when creating copies of packets
>>>> + *
>>>> + * @param pkt_src Source packet
>>>> + * @param pkt_dst Destination packet
>>>> + */
>>>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst);
>>>>
>>>
>>> void odp_packet_copy(odp_packet_t dst, odp_packet_t src);
>>> It should be other way around: dst on left, src on rigth (compared with
>>> memcpy() or x = y;). Could also
>>> return an error if one of the packets are broken and cannot copy.
>>>
>>
>> Ok, I will switch the parameters around. About broken packages, what
>> should I check there, the error flags?
>>
>
> Invalid packets or dst packet smaller than src packet, ...
>
>
>
>>
>>
>>>
>>>
>>>
>>>> +
>>>>  #ifdef __cplusplus
>>>>  }
>>>>  #endif
>>>> diff --git a/platform/linux-generic/source/odp_packet.c
>>>> b/platform/linux-generic/source/odp_packet.c
>>>> index b990222..47cabd3 100644
>>>> --- a/platform/linux-generic/source/odp_packet.c
>>>> +++ b/platform/linux-generic/source/odp_packet.c
>>>> @@ -331,3 +331,27 @@ void odp_packet_print(odp_packet_t pkt)
>>>>          printf("\n%s\n", str);
>>>>  }
>>>>
>>>> +void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst)
>>>> +{
>>>> +        odp_packet_hdr_t *const pkt_hdr_src = odp_packet_hdr(pkt_src);
>>>> +        odp_packet_hdr_t *const pkt_hdr_dst = odp_packet_hdr(pkt_dst);
>>>> +        const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t,
>>>> buf_hdr);
>>>>
>>>
>>> start_offset = 0 ?? Anyway, it should copy the whole packet, including
>>> the buffer header (packet is a buffer).
>>>
>>
>> the buffer header  contains the buffer index in the pool and the
>> originating pool, I suppose those should not be modified. But the start
>> offset is wrong, it should skip the buf header in this case.
>>
>
> Sorry, read that ODP_FIELD_OFFSETOF. Why not use sizeof(odp_buffer_hdr_t).
> Right, all buffer header fields cannot be copied, but e.g. cur_offset
> should be copied and possible scatters.
>

I'm using ODP_FIELD_SIZEOF because I've used odp_packet_init as an example,
where the metadata is initialized (memset) starting after the buf member.
Isn't it ok to leave it like that?

About scatter, those are not used yet, so I guess I would have to create
copies of the scatter buffer list as well. That isn't pretty because it
forces me to copy the entire data in the buffer, without taking into
account the frame_len. I think we should introduce a length member in
odp_buffer_hdr_t very soon, it should come in handy for other things as
well.

I will introduce a dummy function for now, and we will have to make sure to
fill that in at the right time.

>
>
>
>
>>
>>>
>>>> +        odp_pktio_t input;
>>>> +        uint8_t *start_src;
>>>> +        uint8_t *start_dst;
>>>> +        size_t len;
>>>> +
>>>> +        /* Preserve input */
>>>> +        input = pkt_hdr_dst->input;
>>>
>>>
>>> Why this field is not copied over? Dst packet should be exact copy of
>>> the src packet.
>>>
>>
>> I thought the pktio should not be touched. I don't need it in my code so
>> I will give up on it.
>>
>
> input is metadata (the input interface id) and must be copied also (it's
> not odp_packet_copy_except_input_port() :) ).
>

Ok, I will do that.

>
>
>> +
>>>> +        start_src = (uint8_t *)pkt_hdr_src + start_offset;
>>>> +        start_dst = (uint8_t *)pkt_hdr_dst + start_offset;
>>>> +
>>>> +        len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset;
>>>> +        len += pkt_hdr_src->frame_offset + pkt_hdr_src->frame_len;
>>>> +
>>>> +        memcpy(start_dst, start_src, len);
>>>> +
>>>> +        /* Restore input */
>>>> +        pkt_hdr_dst->input = input;
>>>> +}
>>>> diff --git a/test/packet_netmap/odp_example_pktio_netmap.c
>>>> b/test/packet_netmap/odp_example_pktio_netmap.c
>>>> index 2d74f8a..f9ed3b3 100644
>>>> --- a/test/packet_netmap/odp_example_pktio_netmap.c
>>>> +++ b/test/packet_netmap/odp_example_pktio_netmap.c
>>>> @@ -155,20 +155,11 @@ static void *pktio_queue_thread(void *arg)
>>>>
>>>
>
> BTW, there synchronization error here (line before the patched line under):
>
> buf = odp_schedule(NULL);
> pkt = odp_packet_from_buffer(buf);
> ...
> odp_queue_enq(args->thread[pktio_nr].bridge_q, buf);
>

Thanks, I haven't notticed the error.


>
> You enqueued the buf already and with that lost the ownership to it (and
> to pkt). When you copy data from it the receiver may have already
> freed/overwritten the data.
>
> -Petri
>
>
>
>>                  if (args->thread[pktio_nr].netmap_mode ==
>>>> ODP_NETMAP_MODE_HW) {
>>>>                          odp_packet_t pkt_copy;
>>>>                          odp_buffer_t buf_copy;
>>>> -                        size_t frame_len = odp_packet_get_len(pkt);
>>>> -                        size_t l2_offset = odp_packet_l2_offset(pkt);
>>>> -                        size_t l3_offset = odp_packet_l3_offset(pkt);
>>>>
>>>>                          buf_copy = odp_buffer_alloc(pkt_pool);
>>>>                          pkt_copy = odp_packet_from_buffer(buf_copy);
>>>>
>>>> -                        odp_packet_init(pkt_copy);
>>>> -                        odp_packet_set_len(pkt_copy, frame_len);
>>>> -                        odp_packet_set_l2_offset(pkt_copy,
>>>> l2_offset);
>>>> -                        odp_packet_set_l3_offset(pkt_copy,
>>>> l3_offset);
>>>> -
>>>> -                        memcpy(odp_buffer_addr(pkt_copy),
>>>> -                               odp_buffer_addr(pkt), frame_len);
>>>> +                        odp_packet_copy(pkt, pkt_copy);
>>>>
>>>>                          swap_pkt_addrs(&pkt_copy, 1);
>>>>
>>>> --
>>>> 1.8.3.2
>>>>
>>>>
>> /Ciprian
>>
>>  --
> You received this message because you are subscribed to the Google Groups
> "LNG ODP Sub-team - lng-odp@linaro.org" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to lng-odp+unsubscribe@linaro.org.
> To post to this group, send email to lng-odp@linaro.org.
> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/.
> To view this discussion on the web visit
> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/8782e10f-7706-4479-9aab-cc049b6a66ba%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/8782e10f-7706-4479-9aab-cc049b6a66ba%40linaro.org?utm_medium=email&utm_source=footer>
> .
> For more options, visit https://groups.google.com/a/linaro.org/d/optout.
>
diff mbox

Patch

diff --git a/include/odp_packet.h b/include/odp_packet.h
index 3c7c9d0..c0bedcd 100644
--- a/include/odp_packet.h
+++ b/include/odp_packet.h
@@ -209,6 +209,15 @@  void odp_packet_set_l4_offset(odp_packet_t pkt, size_t offset);
  */
 void odp_packet_print(odp_packet_t pkt);
 
+/**
+ * Copy contents and metadata from pkt_src to pkt_dst
+ * Useful when creating copies of packets
+ *
+ * @param pkt_src Source packet
+ * @param pkt_dst Destination packet
+ */
+void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/platform/linux-generic/source/odp_packet.c b/platform/linux-generic/source/odp_packet.c
index b990222..47cabd3 100644
--- a/platform/linux-generic/source/odp_packet.c
+++ b/platform/linux-generic/source/odp_packet.c
@@ -331,3 +331,27 @@  void odp_packet_print(odp_packet_t pkt)
 	printf("\n%s\n", str);
 }
 
+void odp_packet_copy(odp_packet_t pkt_src, odp_packet_t pkt_dst)
+{
+	odp_packet_hdr_t *const pkt_hdr_src = odp_packet_hdr(pkt_src);
+	odp_packet_hdr_t *const pkt_hdr_dst = odp_packet_hdr(pkt_dst);
+	const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr);
+	odp_pktio_t input;
+	uint8_t *start_src;
+	uint8_t *start_dst;
+	size_t len;
+
+	/* Preserve input */
+	input = pkt_hdr_dst->input;
+
+	start_src = (uint8_t *)pkt_hdr_src + start_offset;
+	start_dst = (uint8_t *)pkt_hdr_dst + start_offset;
+
+	len = ODP_OFFSETOF(odp_packet_hdr_t, payload) - start_offset;
+	len += pkt_hdr_src->frame_offset + pkt_hdr_src->frame_len;
+
+	memcpy(start_dst, start_src, len);
+
+	/* Restore input */
+	pkt_hdr_dst->input = input;
+}
diff --git a/test/packet_netmap/odp_example_pktio_netmap.c b/test/packet_netmap/odp_example_pktio_netmap.c
index 2d74f8a..f9ed3b3 100644
--- a/test/packet_netmap/odp_example_pktio_netmap.c
+++ b/test/packet_netmap/odp_example_pktio_netmap.c
@@ -155,20 +155,11 @@  static void *pktio_queue_thread(void *arg)
 		if (args->thread[pktio_nr].netmap_mode == ODP_NETMAP_MODE_HW) {
 			odp_packet_t pkt_copy;
 			odp_buffer_t buf_copy;
-			size_t frame_len = odp_packet_get_len(pkt);
-			size_t l2_offset = odp_packet_l2_offset(pkt);
-			size_t l3_offset = odp_packet_l3_offset(pkt);
 
 			buf_copy = odp_buffer_alloc(pkt_pool);
 			pkt_copy = odp_packet_from_buffer(buf_copy);
 
-			odp_packet_init(pkt_copy);
-			odp_packet_set_len(pkt_copy, frame_len);
-			odp_packet_set_l2_offset(pkt_copy, l2_offset);
-			odp_packet_set_l3_offset(pkt_copy, l3_offset);
-
-			memcpy(odp_buffer_addr(pkt_copy),
-			       odp_buffer_addr(pkt), frame_len);
+			odp_packet_copy(pkt, pkt_copy);
 
 			swap_pkt_addrs(&pkt_copy, 1);