> From: Bruce Richardson [mailto:[email protected]]
> Sent: Friday, 23 January 2026 13.54
>
> On Fri, Jan 23, 2026 at 01:27:54PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:[email protected]]
> > > Sent: Friday, 23 January 2026 13.09
> > >
> > > On Fri, Jan 23, 2026 at 01:05:10PM +0100, Morten Brørup wrote:
> > > > I haven't looked into the details yet, but have a quick question
> > > inline below.
> > > >
> > > > > @@ -345,12 +345,20 @@ ci_txq_release_all_mbufs(struct
> ci_tx_queue
> > > *txq,
> > > > > bool use_ctx)
> > > > > return;
> > > > >
> > > > > if (!txq->vector_tx) {
> > > > > - for (uint16_t i = 0; i < txq->nb_tx_desc; i++) {
> > > > > - if (txq->sw_ring[i].mbuf != NULL) {
> > > >
> > > > You changed this loop to only operate on not-yet-cleaned
> descriptors.
> > > >
> > > > Here comes the first part of my question:
> > > > You removed the NULL check for txq->sw_ring[i].mbuf, thereby
> assuming
> > > that it is never NULL for not-yet-cleaned descriptors.
> > > >
> > >
> > > Good point, I was quite focused on making this block and the vector
> > > block
> > > the same, I forgot that we can have NULL pointers for context
> > > descriptors.
> > > That was a silly mistake (and AI never caught it for me either.)
> > >
> > > > > + /* Free mbufs from (last_desc_cleaned + 1) to
> (tx_tail -
> > > > > 1). */
> > > > > + const uint16_t start = (txq->last_desc_cleaned + 1) %
> txq-
> > > > > >nb_tx_desc;
> > > > > + const uint16_t nb_desc = txq->nb_tx_desc;
> > > > > + const uint16_t end = txq->tx_tail;
> > > > > +
> > > > > + uint16_t i = start;
> >
> > Suggest getting rid of "start"; it is only used for initializing "i".
> >
>
> Not sure it's worth doing. I quite like having an explicit start and
> end
> values for clarity.
I have no preference, and it's a matter of taste, so the choice is yours. :-)
>
> > > > > + if (end < i) {
> > > > > + for (; i < nb_desc; i++)
> > > > > rte_pktmbuf_free_seg(txq-
> >sw_ring[i].mbuf);
> > > > > - txq->sw_ring[i].mbuf = NULL;
> > > > > - }
> > > > > + i = 0;
> > > > > }
> > > > > + for (; i < end; i++)
> > > > > + rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
> > > > > + memset(txq->sw_ring, 0, sizeof(txq->sw_ring[0]) *
> nb_desc);
> >
> > Consider also splitting this memset() into two, one for each of the
> two for loops.
> > Then you might need to keep "start" and make it non-const. :-)
> >
>
> Don't see the point of that. The memset just zeros the whole array,
> ignoring wraparound so no point in doing two memsets when one will do.
>
> > > > > return; }
> >
> > Or just keep the original version, looping over all descriptors.
> >
>
> The reason for this whole change is that after the refactor the old
> code
> was wrong.
>
> The original code used the fact that all mbuf pointers were zereod or
> overwritten after being freed, but that no longer applies, because we
> free
> the mbufs in bulk after we check the dd bits, rather than doing so
> individually later immediately before reuse. Instead, in both the
> datapath
> and this release path, we must use the index values to track what mbuf
> entries are valid or invalid. (We go from having two states, NULL or
> non-NULL, to 3; invalid i.e. freed or NULL, valid-NULL i.e. in slot
> used by
> context descriptor, valid-non-NULL i.e. a pointer to a not-yet-cleaned-
> up
> mbuf).
Thank you for clarifying.
I thought of the two for loops as a kind of performance optimization, skipping
the sub-array of already freed descriptors. In that case, memsetting only the
two remaining sub-arrays might have been a good idea.
That's not the case, so having one memset for the whole array is perfectly fine.
