On Sat, Jan 11, 2025 at 09:07:14AM -0800, Stephen Hemminger wrote:
> On Mon, 6 Jan 2025 14:25:05 +0000
> Bruce Richardson <bruce.richard...@intel.com> wrote:
>
> > On Fri, Dec 20, 2024 at 08:15:40AM -0800, Stephen Hemminger wrote:
> > > On Fri, 20 Dec 2024 14:38:58 +0000
> > > Bruce Richardson <bruce.richard...@intel.com> wrote:
> > >
> > > > +
> > > > + if (!split_flags[buf_idx]) {
> > > > + /* it's the last packet of the set */
> > > > + start->hash = end->hash;
> > > > + start->vlan_tci = end->vlan_tci;
> > > > + start->ol_flags = end->ol_flags;
> > > > + /* we need to strip crc for the whole
> > > > packet */
> > > > + start->pkt_len -= crc_len;
> > > > + if (end->data_len > crc_len) {
> > > > + end->data_len -= crc_len;
> > > > + } else {
> > > > + /* free up last mbuf */
> > > > + struct rte_mbuf *secondlast =
> > > > start;
> > > > +
> > > > + start->nb_segs--;
> > > > + while (secondlast->next != end)
> > > > + secondlast =
> > > > secondlast->next;
> > > > + secondlast->data_len -=
> > > > (crc_len - end->data_len);
> > > > + secondlast->next = NULL;
> > > > + rte_pktmbuf_free_seg(end);
> > > > + }
> > >
> > > The problem with freeing the last buffer is that the CRC will be garbage.
> > > What if the CRC is sitting past the last mbuf?
> > >
> > > +-----------------------+ +-----+
> > > | Data +--->+ CRC |
> > > +-----------------------+ +-----+
> > >
> > > This part (from original code) will free the second mbuf which contains
> > > the CRC. The whole "don't strip CRC and leave it past the mbuf data" model
> > > of mbuf's is a danger trap.
> >
> > Can you explain more clearly what you see as the issue with the current
> > code above?
> >
> > The "crc_len" variable contains the length of the CRC included in the
> > packet, which should be removed from that before returning the mbuf from RX.
> > It contains "0" if the CRC is HW stripped, and "4" if the CRC needs to be
> > removed by software - something that is done just by subtracting the CRC
> > length from the packet and buffer lengths. The freeing of the last segment
> > occurs only in the case that the last segment contains the CRC only - or
> > part of the CRC only, as otherwise we would have an extra empty buffer at
> > the end of the chain.
>
> But if the application is using the driver in "keep CRC" mode,
> then it probably intends to look at the CRC. In that case crc_len is 4,
> and that trailing buffer may need to be retained.
>
> It all goes back to the design mistake of the semantics of "keep CRC"
> mode. In that mode, the mbuf chain has hidden data (past pkt_len and
> data_len).
>
If CRC is not to be stripped, then crc_len should be zero here also, in
which case nothing happens for freeing data. [If crc_len is not zero in
that case, that's a bug in the config path, not datapath here]
/Bruce
