| Message ID | 1512734518-103757-3-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 2/9] dp-packet: fix reset_packet for multi-seg > mbufs > > When adjusting the size of a dp_packet, dp_packet_set_data() should be invoked > before dp_packet_set_size(),since for DPDK multi-segment mbufs, the former > will use the segments's data_off and buf_len to derive the frame size that should > be set (this behaviour is introduced in a subsequent commit). > > Currently, in dp_packet_reset_packet(), that order is reversed. > Swap the order of same to resolve. > > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > --- > lib/dp-packet.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index b4b721c..47502ad 100644 > --- a/lib/dp-packet.h > +++ b/lib/dp-packet.h > @@ -569,8 +569,8 @@ dp_packet_set_data(struct dp_packet *b, void *data) > static inline void dp_packet_reset_packet(struct dp_packet *b, int off) { > - dp_packet_set_size(b, dp_packet_size(b) - off); > dp_packet_set_data(b, ((unsigned char *) dp_packet_data(b) + off)); > + dp_packet_set_size(b, dp_packet_size(b) - off); [Sugesh] With the subsequent commit changes, this going to be bit difficult to make sure the packet size is set properly. Is is possible to keep these two function to be indepedant as before? I can see many places these functions are get called independently. So in the packet processing pipeline, its likely that set_size function get invoked without set_data that may cause the wrong segment size. It would be great if we can mandate in the code that the offsets are set correctly before setting the packet size each time? What do you think? Do you have any suggestion on this? If there are no way, atleast this has to be commented in the code. > dp_packet_reset_offsets(b); > } > > -- > 1.9.3
>From: Chandran, Sugesh >Sent: Friday, December 8, 2017 5:55 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 2/9] dp-packet: fix reset_packet for >multi-seg mbufs > > > >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 2/9] dp-packet: fix reset_packet for multi- >seg >> mbufs >> >> When adjusting the size of a dp_packet, dp_packet_set_data() should be >invoked >> before dp_packet_set_size(),since for DPDK multi-segment mbufs, the former >> will use the segments's data_off and buf_len to derive the frame size that >should >> be set (this behaviour is introduced in a subsequent commit). >> >> Currently, in dp_packet_reset_packet(), that order is reversed. >> Swap the order of same to resolve. >> >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >> --- >> lib/dp-packet.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index b4b721c..47502ad 100644 >> --- a/lib/dp-packet.h >> +++ b/lib/dp-packet.h >> @@ -569,8 +569,8 @@ dp_packet_set_data(struct dp_packet *b, void *data) >> static inline void dp_packet_reset_packet(struct dp_packet *b, int off) { >> - dp_packet_set_size(b, dp_packet_size(b) - off); >> dp_packet_set_data(b, ((unsigned char *) dp_packet_data(b) + off)); >> + dp_packet_set_size(b, dp_packet_size(b) - off); >[Sugesh] With the subsequent commit changes, this going to be bit difficult to >make sure the packet size is set properly. >Is is possible to keep these two function to be indepedant as before? >I can see many places these functions are get called independently. So in the >packet processing pipeline, its likely that > set_size function get invoked without set_data that may cause the wrong >segment size. It would be great if we can mandate in the code that the offsets >are >set correctly before setting the packet size each time? What do you think? Do >you have any suggestion on this? If there are no way, atleast this has to be >commented in the code. That's a fair point Sugesh; I'll take a look and see what I can do in the next version. Thanks, Mark >> dp_packet_reset_offsets(b); >> } >> >> -- >> 1.9.3
diff --git a/lib/dp-packet.h b/lib/dp-packet.h index b4b721c..47502ad 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -569,8 +569,8 @@ dp_packet_set_data(struct dp_packet *b, void *data) static inline void dp_packet_reset_packet(struct dp_packet *b, int off) { - dp_packet_set_size(b, dp_packet_size(b) - off); dp_packet_set_data(b, ((unsigned char *) dp_packet_data(b) + off)); + dp_packet_set_size(b, dp_packet_size(b) - off); dp_packet_reset_offsets(b); }
When adjusting the size of a dp_packet, dp_packet_set_data() should be invoked before dp_packet_set_size(),since for DPDK multi-segment mbufs, the former will use the segments's data_off and buf_len to derive the frame size that should be set (this behaviour is introduced in a subsequent commit). Currently, in dp_packet_reset_packet(), that order is reversed. Swap the order of same to resolve. Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> --- lib/dp-packet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)