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 |
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
>-----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
>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
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 --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++; }