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.
> > > > + 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).
/Bruce
