Regards _Sugesh
> -----Original Message----- > From: Kavanagh, Mark B > Sent: Tuesday, December 12, 2017 12:10 PM > To: Chandran, Sugesh <[email protected]>; [email protected]; > [email protected] > Cc: Stokes, Ian <[email protected]>; Loftus, Ciara > <[email protected]>; > [email protected] > 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 <[email protected]>; [email protected]; > >[email protected] > >Cc: Stokes, Ian <[email protected]>; Loftus, Ciara > ><[email protected]>; [email protected] > >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: [email protected]; [email protected] > >> Cc: Stokes, Ian <[email protected]>; Loftus, Ciara > ><[email protected]>; > >> [email protected]; Chandran, Sugesh > >> <[email protected]>; Kavanagh, Mark B > >> <[email protected]> > >> 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 <[email protected]> > >> --- > >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
