diff mbox series

[ovs-dev,RFC,V4,6/9] dp-packet: copy mbuf info for packet copy

Message ID 1512734518-103757-7-git-send-email-mark.b.kavanagh@intel.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series netdev-dpdk: support multi-segment mbufs | expand

Commit Message

Mark Kavanagh Dec. 8, 2017, 12:01 p.m. UTC 
From: Michael Qiu <qiudayu@chinac.com>

Currently, when doing packet copy, lots of DPDK mbuf's info
will be missed, like packet type, ol_flags, etc.
Those information is very important for DPDK to do
packets processing.

Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
[mark.b.kavanagh@intel.com rebased]

Signed-off-by: Michael Qiu <qiudayu@chinac.com>
Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
---
 lib/dp-packet.c   | 3 +++
 lib/netdev-dpdk.c | 4 ++++
 2 files changed, 7 insertions(+)

Comments

Chandran, Sugesh Dec. 8, 2017, 6:01 p.m. UTC | #1 
Regards
_Sugesh


> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, December 8, 2017 12:02 PM
> To: dev@openvswitch.org; qiudayu@chinac.com
> Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
> santosh.shukla@caviumnetworks.com; Chandran, Sugesh
> <sugesh.chandran@intel.com>; Kavanagh, Mark B
> <mark.b.kavanagh@intel.com>
> Subject: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
> copy
> 
> From: Michael Qiu <qiudayu@chinac.com>
> 
> Currently, when doing packet copy, lots of DPDK mbuf's info will be missed, like
> packet type, ol_flags, etc.
> Those information is very important for DPDK to do packets processing.
> 
> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> [mark.b.kavanagh@intel.com rebased]
> 
> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> ---
>  lib/dp-packet.c   | 3 +++
>  lib/netdev-dpdk.c | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ad71486..d628037 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -178,6 +178,9 @@ dp_packet_clone_with_headroom(const struct
> dp_packet *buffer, size_t headroom)
> 
>  #ifdef DPDK_NETDEV
>      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> +    new_buffer->mbuf.tx_offload = buffer->mbuf.tx_offload;
> +    new_buffer->mbuf.packet_type = buffer->mbuf.packet_type;
> +    new_buffer->mbuf.nb_segs = buffer->mbuf.nb_segs;
[Sugesh] This function get lot many #if, #else with DPDK. It must need a cleanup.
What do you think? 
Also will it would better if we keep all the mbuf field copy into a different function, something like
copy_dpdk_mbuf_flags(..)? 

>  #else
>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;  #endif diff --git
> a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4167497..8a81690 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1866,6 +1866,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,
> struct dp_packet_batch *batch)
>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>                 dp_packet_data(packet), size);
>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
> +        pkts[txcnt]->nb_segs = packet->mbuf.nb_segs;
> +        pkts[txcnt]->ol_flags = packet->mbuf.ol_flags;
> +        pkts[txcnt]->packet_type = packet->mbuf.packet_type;
> +        pkts[txcnt]->tx_offload = packet->mbuf.tx_offload;
> 
>          txcnt++;
>      }
> --
> 1.9.3
Mark Kavanagh Dec. 12, 2017, 1:17 p.m. UTC | #2 
>-----Original Message-----
>From: Chandran, Sugesh
>Sent: Friday, December 8, 2017 6:01 PM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org;
>qiudayu@chinac.com
>Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
><ciara.loftus@intel.com>; santosh.shukla@caviumnetworks.com
>Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
>copy
>
>
>
>Regards
>_Sugesh
>
>
>> -----Original Message-----
>> From: Kavanagh, Mark B
>> Sent: Friday, December 8, 2017 12:02 PM
>> To: dev@openvswitch.org; qiudayu@chinac.com
>> Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
><ciara.loftus@intel.com>;
>> santosh.shukla@caviumnetworks.com; Chandran, Sugesh
>> <sugesh.chandran@intel.com>; Kavanagh, Mark B
>> <mark.b.kavanagh@intel.com>
>> Subject: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
>> copy
>>
>> From: Michael Qiu <qiudayu@chinac.com>
>>
>> Currently, when doing packet copy, lots of DPDK mbuf's info will be missed,
>like
>> packet type, ol_flags, etc.
>> Those information is very important for DPDK to do packets processing.
>>
>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> [mark.b.kavanagh@intel.com rebased]
>>
>> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> ---
>>  lib/dp-packet.c   | 3 +++
>>  lib/netdev-dpdk.c | 4 ++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ad71486..d628037 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -178,6 +178,9 @@ dp_packet_clone_with_headroom(const struct
>> dp_packet *buffer, size_t headroom)
>>
>>  #ifdef DPDK_NETDEV
>>      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> +    new_buffer->mbuf.tx_offload = buffer->mbuf.tx_offload;
>> +    new_buffer->mbuf.packet_type = buffer->mbuf.packet_type;
>> +    new_buffer->mbuf.nb_segs = buffer->mbuf.nb_segs;
>[Sugesh] This function get lot many #if, #else with DPDK. It must need a
>cleanup.
>What do you think?

During implementation, I had thought about separating out the code into two separate dp_packet_clone_headroom() functions: DPDK-based, and non-DPDK-based, within compiler guards. This would improve readability, but at the expense of repeated code; I'll do this in the next version, and see how it is received.
 
>Also will it would better if we keep all the mbuf field copy into a different
>function, something like
>copy_dpdk_mbuf_flags(..)?

Do you think that is still warranted if there are two separate dp_packet_clone_with_headroom() functions, as previously described?

Thanks,
Mark

>
>>  #else
>>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;  #endif diff --git
>> a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4167497..8a81690 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1866,6 +1866,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,
>> struct dp_packet_batch *batch)
>>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>>                 dp_packet_data(packet), size);
>>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
>> +        pkts[txcnt]->nb_segs = packet->mbuf.nb_segs;
>> +        pkts[txcnt]->ol_flags = packet->mbuf.ol_flags;
>> +        pkts[txcnt]->packet_type = packet->mbuf.packet_type;
>> +        pkts[txcnt]->tx_offload = packet->mbuf.tx_offload;
>>
>>          txcnt++;
>>      }
>> --
>> 1.9.3
Mark Kavanagh Dec. 12, 2017, 2:48 p.m. UTC | #3 
>From: Kavanagh, Mark B
>Sent: Tuesday, December 12, 2017 1:18 PM
>To: Chandran, Sugesh <sugesh.chandran@intel.com>; dev@openvswitch.org;
>qiudayu@chinac.com
>Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
><ciara.loftus@intel.com>; santosh.shukla@caviumnetworks.com
>Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
>copy
>
>
>
>>-----Original Message-----
>>From: Chandran, Sugesh
>>Sent: Friday, December 8, 2017 6:01 PM
>>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org;
>>qiudayu@chinac.com
>>Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
>><ciara.loftus@intel.com>; santosh.shukla@caviumnetworks.com
>>Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
>>copy
>>
>>
>>
>>Regards
>>_Sugesh
>>
>>
>>> -----Original Message-----
>>> From: Kavanagh, Mark B
>>> Sent: Friday, December 8, 2017 12:02 PM
>>> To: dev@openvswitch.org; qiudayu@chinac.com
>>> Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
>><ciara.loftus@intel.com>;
>>> santosh.shukla@caviumnetworks.com; Chandran, Sugesh
>>> <sugesh.chandran@intel.com>; Kavanagh, Mark B
>>> <mark.b.kavanagh@intel.com>
>>> Subject: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
>>> copy
>>>
>>> From: Michael Qiu <qiudayu@chinac.com>
>>>
>>> Currently, when doing packet copy, lots of DPDK mbuf's info will be missed,
>>like
>>> packet type, ol_flags, etc.
>>> Those information is very important for DPDK to do packets processing.
>>>
>>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>> [mark.b.kavanagh@intel.com rebased]
>>>
>>> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
>>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>> ---
>>>  lib/dp-packet.c   | 3 +++
>>>  lib/netdev-dpdk.c | 4 ++++
>>>  2 files changed, 7 insertions(+)
>>>
>>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ad71486..d628037
>100644
>>> --- a/lib/dp-packet.c
>>> +++ b/lib/dp-packet.c
>>> @@ -178,6 +178,9 @@ dp_packet_clone_with_headroom(const struct
>>> dp_packet *buffer, size_t headroom)
>>>
>>>  #ifdef DPDK_NETDEV
>>>      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>>> +    new_buffer->mbuf.tx_offload = buffer->mbuf.tx_offload;
>>> +    new_buffer->mbuf.packet_type = buffer->mbuf.packet_type;
>>> +    new_buffer->mbuf.nb_segs = buffer->mbuf.nb_segs;
>>[Sugesh] This function get lot many #if, #else with DPDK. It must need a
>>cleanup.
>>What do you think?
>
>During implementation, I had thought about separating out the code into two
>separate dp_packet_clone_headroom() functions: DPDK-based, and non-DPDK-based,
>within compiler guards. This would improve readability, but at the expense of
>repeated code; I'll do this in the next version, and see how it is received.
>
>>Also will it would better if we keep all the mbuf field copy into a different
>>function, something like
>>copy_dpdk_mbuf_flags(..)?
>
>Do you think that is still warranted if there are two separate
>dp_packet_clone_with_headroom() functions, as previously described?
>

Edit: I'll add a function for this, since the same code is repeated in dpdk_do_tx_copy().
-Mark

>Thanks,
>Mark
>
>>
>>>  #else
>>>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;  #endif diff --
>git
>>> a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4167497..8a81690 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -1866,6 +1866,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,
>>> struct dp_packet_batch *batch)
>>>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>>>                 dp_packet_data(packet), size);
>>>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
>>> +        pkts[txcnt]->nb_segs = packet->mbuf.nb_segs;
>>> +        pkts[txcnt]->ol_flags = packet->mbuf.ol_flags;
>>> +        pkts[txcnt]->packet_type = packet->mbuf.packet_type;
>>> +        pkts[txcnt]->tx_offload = packet->mbuf.tx_offload;
>>>
>>>          txcnt++;
>>>      }
>>> --
>>> 1.9.3
Chandran, Sugesh Dec. 12, 2017, 8:32 p.m. UTC | #4 
Regards
_Sugesh


> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Tuesday, December 12, 2017 2:49 PM
> To: Chandran, Sugesh <sugesh.chandran@intel.com>; dev@openvswitch.org;
> qiudayu@chinac.com
> Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
> santosh.shukla@caviumnetworks.com
> Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for packet
> copy
> 
> >From: Kavanagh, Mark B
> >Sent: Tuesday, December 12, 2017 1:18 PM
> >To: Chandran, Sugesh <sugesh.chandran@intel.com>; dev@openvswitch.org;
> >qiudayu@chinac.com
> >Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
> ><ciara.loftus@intel.com>; santosh.shukla@caviumnetworks.com
> >Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for
> >packet copy
> >
> >
> >
> >>-----Original Message-----
> >>From: Chandran, Sugesh
> >>Sent: Friday, December 8, 2017 6:01 PM
> >>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>;
> dev@openvswitch.org;
> >>qiudayu@chinac.com
> >>Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
> >><ciara.loftus@intel.com>; santosh.shukla@caviumnetworks.com
> >>Subject: RE: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for
> >>packet copy
> >>
> >>
> >>
> >>Regards
> >>_Sugesh
> >>
> >>
> >>> -----Original Message-----
> >>> From: Kavanagh, Mark B
> >>> Sent: Friday, December 8, 2017 12:02 PM
> >>> To: dev@openvswitch.org; qiudayu@chinac.com
> >>> Cc: Stokes, Ian <ian.stokes@intel.com>; Loftus, Ciara
> >><ciara.loftus@intel.com>;
> >>> santosh.shukla@caviumnetworks.com; Chandran, Sugesh
> >>> <sugesh.chandran@intel.com>; Kavanagh, Mark B
> >>> <mark.b.kavanagh@intel.com>
> >>> Subject: [ovs-dev][RFC PATCH V4 6/9] dp-packet: copy mbuf info for
> >>> packet copy
> >>>
> >>> From: Michael Qiu <qiudayu@chinac.com>
> >>>
> >>> Currently, when doing packet copy, lots of DPDK mbuf's info will be
> >>> missed,
> >>like
> >>> packet type, ol_flags, etc.
> >>> Those information is very important for DPDK to do packets processing.
> >>>
> >>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> >>> [mark.b.kavanagh@intel.com rebased]
> >>>
> >>> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
> >>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> >>> ---
> >>>  lib/dp-packet.c   | 3 +++
> >>>  lib/netdev-dpdk.c | 4 ++++
> >>>  2 files changed, 7 insertions(+)
> >>>
> >>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index
> >>> ad71486..d628037
> >100644
> >>> --- a/lib/dp-packet.c
> >>> +++ b/lib/dp-packet.c
> >>> @@ -178,6 +178,9 @@ dp_packet_clone_with_headroom(const struct
> >>> dp_packet *buffer, size_t headroom)
> >>>
> >>>  #ifdef DPDK_NETDEV
> >>>      new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> >>> +    new_buffer->mbuf.tx_offload = buffer->mbuf.tx_offload;
> >>> +    new_buffer->mbuf.packet_type = buffer->mbuf.packet_type;
> >>> +    new_buffer->mbuf.nb_segs = buffer->mbuf.nb_segs;
> >>[Sugesh] This function get lot many #if, #else with DPDK. It must need
> >>a cleanup.
> >>What do you think?
> >
> >During implementation, I had thought about separating out the code into
> >two separate dp_packet_clone_headroom() functions: DPDK-based, and
> >non-DPDK-based, within compiler guards. This would improve readability,
> >but at the expense of repeated code; I'll do this in the next version, and see
> how it is received.
> >
> >>Also will it would better if we keep all the mbuf field copy into a
> >>different function, something like copy_dpdk_mbuf_flags(..)?
> >
> >Do you think that is still warranted if there are two separate
> >dp_packet_clone_with_headroom() functions, as previously described?
> >
> 
> Edit: I'll add a function for this, since the same code is repeated in
> dpdk_do_tx_copy().
[Sugesh] sure,thank you.

> -Mark
> 
> >Thanks,
> >Mark
> >
> >>
> >>>  #else
> >>>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;  #endif
> >>> diff --
> >git
> >>> a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4167497..8a81690
> >>> 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -1866,6 +1866,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int
> >>> qid, struct dp_packet_batch *batch)
> >>>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
> >>>                 dp_packet_data(packet), size);
> >>>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
> >>> +        pkts[txcnt]->nb_segs = packet->mbuf.nb_segs;
> >>> +        pkts[txcnt]->ol_flags = packet->mbuf.ol_flags;
> >>> +        pkts[txcnt]->packet_type = packet->mbuf.packet_type;
> >>> +        pkts[txcnt]->tx_offload = packet->mbuf.tx_offload;
> >>>
> >>>          txcnt++;
> >>>      }
> >>> --
> >>> 1.9.3
diff mbox series

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ad71486..d628037 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -178,6 +178,9 @@  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
 
 #ifdef DPDK_NETDEV
     new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
+    new_buffer->mbuf.tx_offload = buffer->mbuf.tx_offload;
+    new_buffer->mbuf.packet_type = buffer->mbuf.packet_type;
+    new_buffer->mbuf.nb_segs = buffer->mbuf.nb_segs;
 #else
     new_buffer->rss_hash_valid = buffer->rss_hash_valid;
 #endif
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4167497..8a81690 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1866,6 +1866,10 @@  dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
         memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
                dp_packet_data(packet), size);
         dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
+        pkts[txcnt]->nb_segs = packet->mbuf.nb_segs;
+        pkts[txcnt]->ol_flags = packet->mbuf.ol_flags;
+        pkts[txcnt]->packet_type = packet->mbuf.packet_type;
+        pkts[txcnt]->tx_offload = packet->mbuf.tx_offload;
 
         txcnt++;
     }