On Mon, 13 Jan 2025 10:04:50 +0000
Bruce Richardson <bruce.richard...@intel.com> wrote:
> 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
Ok, that makes sense.
