[ovs-dev] [PATCH v10 05/14] dp-packet: Fix data_len handling multi-seg mbufs.
fbl at sysclose.org
Fri Oct 5 16:49:40 UTC 2018
On Fri, Oct 05, 2018 at 03:50:46PM +0100, Lam, Tiago wrote:
> On 03/10/2018 19:26, Flavio Leitner wrote:
> > On Fri, Sep 28, 2018 at 05:15:06PM +0100, Tiago Lam wrote:
> >> When a dp_packet is from a DPDK source, and it contains multi-segment
> >> mbufs, the data_len is not equal to the packet size, pkt_len. Instead,
> >> the data_len of each mbuf in the chain should be considered while
> >> distributing the new (provided) size.
> >> To account for the above dp_packet_set_size() has been changed so that,
> >> in the multi-segment mbufs case, only the data_len on the last mbuf of
> >> the chain and the total size of the packet, pkt_len, are changed. The
> >> data_len on the intermediate mbufs preceeding the last mbuf is not
> >> changed by dp_packet_set_size(). Furthermore, in some cases
> >> dp_packet_set_size() may be used to set a smaller size than the current
> >> packet size, thus effectively trimming the end of the packet. In the
> >> multi-segment mbufs case this may lead to lingering mbufs that may need
> >> freeing.
> >> __dp_packet_set_data() now also updates an mbufs' data_len after setting
> >> the data offset. This is so that both fields are always in sync for each
> >> mbuf in a chain.
> > I think we need an assert(source) to make sure dp_packet_shift() will
> > never be called for a multi-seg.
> I guess you made this comment before seeing patch 07/14 where a
> dp_packet_shift() implementation is introduced for DPDK packets? But
> nonetheless, it has been mentioned before by Darrell that it might not
> be worth implementing. Given that the only users for now are in
> lib/pcap-file.c and lib/ovs-ofctl.c, where packets are always of
> DPBUF_MALLOC type, I guess it could make sense to drop the
> implementation. It would certainly help reduce the footprint of this
> patchset for now. If a use case comes for that later on we can always
> introduce it then.
Sure, my point is that it doesn't expect anything other than
DPBUF_MALLOC type, so adding an assert() would be helpful to
catch bugs, that's all.
More information about the dev