| Message ID | 1512734518-103757-4-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 3/9] dp-packet: fix put_uninit for multi-seg > mbufs > > dp_packet_put_uninit(dp_packet, size) appends 'size' bytes to the tail of a > dp_packet. In the case of multi-segment mbufs, it is the data length of the last > mbuf in the mbuf chain that should be adjusted by 'size' bytes. > > In its current implementation, dp_packet_put_uninit() adjusts the dp_packet's > size via a call to dp_packet_set_size(); however, this adjusts the data length of > the first mbuf in the chain, which is incorrect in the case of multi-segment > mbufs. Instead, traverse the mbuf chain to locate the final mbuf of said chain, > and update its data_len[1]. To finish, increase the packet length of the entire > mbuf[2] by 'size'. > > [1] In the case of a single-segment mbuf, this is the mbuf itself. > [2] This is stored in the first mbuf of an mbuf chain. > > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > --- > lib/dp-packet.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..ad71486 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -322,7 +322,27 @@ dp_packet_put_uninit(struct dp_packet *b, size_t size) > void *p; > dp_packet_prealloc_tailroom(b, size); > p = dp_packet_tail(b); > +#ifdef DPDK_NETDEV > + if (b->source == DPBUF_DPDK) { > + struct rte_mbuf *buf = &(b->mbuf); > + /* In the case of multi-segment mbufs, the data length of the last mbuf > + * should be adjusted by 'size' bytes. A call to dp_packet_size() would > + * adjust the data length of the first mbuf in the segment, so we avoid > + * invoking same; as a result, the packet length of the entire mbuf > + * chain (stored in the first mbuf of said chain) must be adjusted here > + * instead. > + */ > + while (buf->next) { > + buf = buf->next; > + } > + buf->data_len += size; [Sugesh] Just to confirm, should we have enough room in buf to accommodate the new size? If not, what will happen? Will it allocate a new mbuf at last? Do you think it's a valid case to happen? My understanding is always mbuf data are over-provisioned previously. The multiple mbuf case, we are allocating the exact size of packet. Wondering if it make any issues? Please ignore this if its not a valid case at all. > + b->mbuf.pkt_len += size; > + } else { > +#endif > dp_packet_set_size(b, dp_packet_size(b) + size); > +#ifdef DPDK_NETDEV > + } > +#endif > return p; > } > > -- > 1.9.3
>From: Chandran, Sugesh >Sent: Friday, December 8, 2017 6:00 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 3/9] dp-packet: fix put_uninit 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 3/9] dp-packet: fix put_uninit for multi-seg >> mbufs >> >> dp_packet_put_uninit(dp_packet, size) appends 'size' bytes to the tail of a >> dp_packet. In the case of multi-segment mbufs, it is the data length of the >last >> mbuf in the mbuf chain that should be adjusted by 'size' bytes. >> >> In its current implementation, dp_packet_put_uninit() adjusts the >dp_packet's >> size via a call to dp_packet_set_size(); however, this adjusts the data >length of >> the first mbuf in the chain, which is incorrect in the case of multi-segment >> mbufs. Instead, traverse the mbuf chain to locate the final mbuf of said >chain, >> and update its data_len[1]. To finish, increase the packet length of the >entire >> mbuf[2] by 'size'. >> >> [1] In the case of a single-segment mbuf, this is the mbuf itself. >> [2] This is stored in the first mbuf of an mbuf chain. >> >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >> --- >> lib/dp-packet.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..ad71486 100644 >> --- a/lib/dp-packet.c >> +++ b/lib/dp-packet.c >> @@ -322,7 +322,27 @@ dp_packet_put_uninit(struct dp_packet *b, size_t size) >> void *p; >> dp_packet_prealloc_tailroom(b, size); >> p = dp_packet_tail(b); >> +#ifdef DPDK_NETDEV >> + if (b->source == DPBUF_DPDK) { >> + struct rte_mbuf *buf = &(b->mbuf); >> + /* In the case of multi-segment mbufs, the data length of the last >mbuf >> + * should be adjusted by 'size' bytes. A call to dp_packet_size() >would >> + * adjust the data length of the first mbuf in the segment, so we >avoid >> + * invoking same; as a result, the packet length of the entire mbuf >> + * chain (stored in the first mbuf of said chain) must be adjusted >here >> + * instead. >> + */ >> + while (buf->next) { >> + buf = buf->next; >> + } >> + buf->data_len += size; >[Sugesh] Just to confirm, should we have enough room in buf to accommodate the >new size? If not, what will happen? dp_packet_prealloc_tailroom() checks that there is enough tailroom in the mbuf to increase the size accordingly, and reallocates the dp_packet if not (if the dp_packet's memory source is not DPDK). However, if the packet memory for the dp_packet comes from DPDK, then dp_packet_resize() triggers an abort(); this behavior already exists in OvS, and this patchset does not change that. Your question has flagged to me however, that some additional functions may need to be modified to accommodate multi-segment mbufs (dp_packet_end(), dp_packet_tail(), for instance). Thanks, Mark >Will it allocate a new mbuf at last? Do you think it's a valid case to happen? >My understanding is always mbuf data are over-provisioned previously. >The multiple mbuf case, we are allocating the exact size of packet. Wondering >if it make any issues? Please ignore this if its not a valid case at all. > >> + b->mbuf.pkt_len += size; >> + } else { >> +#endif >> dp_packet_set_size(b, dp_packet_size(b) + size); >> +#ifdef DPDK_NETDEV >> + } >> +#endif >> return p; >> } >> >> -- >> 1.9.3
Regards _Sugesh > -----Original Message----- > From: Kavanagh, Mark B > Sent: Tuesday, December 12, 2017 12:10 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 3/9] dp-packet: fix put_uninit for multi-seg > mbufs > > >From: Chandran, Sugesh > >Sent: Friday, December 8, 2017 6:00 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 3/9] dp-packet: fix put_uninit 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 3/9] dp-packet: fix put_uninit for > >> multi-seg mbufs > >> > >> dp_packet_put_uninit(dp_packet, size) appends 'size' bytes to the > >> tail of a dp_packet. In the case of multi-segment mbufs, it is the > >> data length of the > >last > >> mbuf in the mbuf chain that should be adjusted by 'size' bytes. > >> > >> In its current implementation, dp_packet_put_uninit() adjusts the > >dp_packet's > >> size via a call to dp_packet_set_size(); however, this adjusts the > >> data > >length of > >> the first mbuf in the chain, which is incorrect in the case of > >> multi-segment mbufs. Instead, traverse the mbuf chain to locate the > >> final mbuf of said > >chain, > >> and update its data_len[1]. To finish, increase the packet length of > >> the > >entire > >> mbuf[2] by 'size'. > >> > >> [1] In the case of a single-segment mbuf, this is the mbuf itself. > >> [2] This is stored in the first mbuf of an mbuf chain. > >> > >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > >> --- > >> lib/dp-packet.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..ad71486 > >> 100644 > >> --- a/lib/dp-packet.c > >> +++ b/lib/dp-packet.c > >> @@ -322,7 +322,27 @@ dp_packet_put_uninit(struct dp_packet *b, size_t > size) > >> void *p; > >> dp_packet_prealloc_tailroom(b, size); > >> p = dp_packet_tail(b); > >> +#ifdef DPDK_NETDEV > >> + if (b->source == DPBUF_DPDK) { > >> + struct rte_mbuf *buf = &(b->mbuf); > >> + /* In the case of multi-segment mbufs, the data length of > >> +the last > >mbuf > >> + * should be adjusted by 'size' bytes. A call to > >> + dp_packet_size() > >would > >> + * adjust the data length of the first mbuf in the segment, > >> + so we > >avoid > >> + * invoking same; as a result, the packet length of the entire mbuf > >> + * chain (stored in the first mbuf of said chain) must be > >> + adjusted > >here > >> + * instead. > >> + */ > >> + while (buf->next) { > >> + buf = buf->next; > >> + } > >> + buf->data_len += size; > >[Sugesh] Just to confirm, should we have enough room in buf to > >accommodate the new size? If not, what will happen? > > dp_packet_prealloc_tailroom() checks that there is enough tailroom in the mbuf > to increase the size accordingly, and reallocates the dp_packet if not (if the > dp_packet's memory source is not DPDK). > However, if the packet memory for the dp_packet comes from DPDK, then > dp_packet_resize() triggers an abort(); this behavior already exists in OvS, and > this patchset does not change that. [Sugesh] I assume the resize was an abort() earlier as we were using single mbuf before. In a multi-seg , cant we avoid this abort(). Looks to me that's one of the function to use multi-seg? What do you think? > > Your question has flagged to me however, that some additional functions may > need to be modified to accommodate multi-segment mbufs (dp_packet_end(), > dp_packet_tail(), for instance). [Sugesh] Sure. > > Thanks, > Mark > > >Will it allocate a new mbuf at last? Do you think it's a valid case to happen? > >My understanding is always mbuf data are over-provisioned previously. > >The multiple mbuf case, we are allocating the exact size of packet. > >Wondering if it make any issues? Please ignore this if its not a valid case at all. > > > >> + b->mbuf.pkt_len += size; > >> + } else { > >> +#endif > >> dp_packet_set_size(b, dp_packet_size(b) + size); > >> +#ifdef DPDK_NETDEV > >> + } > >> +#endif > >> return p; > >> } > >> > >> -- > >> 1.9.3
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..ad71486 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -322,7 +322,27 @@ dp_packet_put_uninit(struct dp_packet *b, size_t size) void *p; dp_packet_prealloc_tailroom(b, size); p = dp_packet_tail(b); +#ifdef DPDK_NETDEV + if (b->source == DPBUF_DPDK) { + struct rte_mbuf *buf = &(b->mbuf); + /* In the case of multi-segment mbufs, the data length of the last mbuf + * should be adjusted by 'size' bytes. A call to dp_packet_size() would + * adjust the data length of the first mbuf in the segment, so we avoid + * invoking same; as a result, the packet length of the entire mbuf + * chain (stored in the first mbuf of said chain) must be adjusted here + * instead. + */ + while (buf->next) { + buf = buf->next; + } + buf->data_len += size; + b->mbuf.pkt_len += size; + } else { +#endif dp_packet_set_size(b, dp_packet_size(b) + size); +#ifdef DPDK_NETDEV + } +#endif return p; }
dp_packet_put_uninit(dp_packet, size) appends 'size' bytes to the tail of a dp_packet. In the case of multi-segment mbufs, it is the data length of the last mbuf in the mbuf chain that should be adjusted by 'size' bytes. In its current implementation, dp_packet_put_uninit() adjusts the dp_packet's size via a call to dp_packet_set_size(); however, this adjusts the data length of the first mbuf in the chain, which is incorrect in the case of multi-segment mbufs. Instead, traverse the mbuf chain to locate the final mbuf of said chain, and update its data_len[1]. To finish, increase the packet length of the entire mbuf[2] by 'size'. [1] In the case of a single-segment mbuf, this is the mbuf itself. [2] This is stored in the first mbuf of an mbuf chain. Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> --- lib/dp-packet.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)