Handle case where mbuf == &txq when IFC_QFLUSH is set

[shurd]

Summary

The ring can have a pointer to txq in it instead of an mbuf.
When this happens, we absolutely don't want to pass it to m_free().

[shurd]

Fix test for txq, and cast to the appropriate type before comparison

[shurd]

mp_ring->items is void *, not struct mbuf *.

[gallatin]

What keeps the normal, non-IFC_QFLUSH from hitting this condition? Is the txq pointer included in r->size?

[shurd]

In D16882#361076, @gallatin wrote:

What keeps the normal, non-IFC_QFLUSH from hitting this condition? Is the txq pointer included in r->size?

The check at line 3671 prevents it, and yes, the txq pointer is added to the ring just as though it's an mbuf, so anything looking at the ring contents needs to test against txq before assuming it's an mbuf, and it's included in r->size.

[gallatin]

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

[shurd]

In D16882#361193, @gallatin wrote:

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

As I recall, it's used to wake up the ring to send laggards. Basically, if you add items to the mp ring while there's an active consumer, those items won't be sent until the next enqueue(). Since the time to the next packet is unbounded, the txq pointer is passed to enqueue() so they get sent at that point. I suspect txq was used simply because it's a non-NULL pointer we have laying around that we know will never be a valid mbuf. @mmacy may be able to add more colour to that.

It's one of those things that is on the Big List of Things To Change.

[mmacy]

In D16882#361200, @shurd wrote:

In D16882#361193, @gallatin wrote:

Sorry, having the ring pointer in the ring is just so odd.. Can you explain why it is there? Maybe it would be better to just not have the ring pointer in the ring and not have to have all these special cases.

I'm fighting an iflib bug myself, and every time I have to dig into the code, i'm irritated at what seems to be complexity driven by premature optimization...

As I recall, it's used to wake up the ring to send laggards. Basically, if you add items to the mp ring while there's an active consumer, those items won't be sent until the next enqueue(). Since the time to the next packet is unbounded, the txq pointer is passed to enqueue() so they get sent at that point. I suspect txq was used simply because it's a non-NULL pointer we have laying around that we know will never be a valid mbuf. @mmacy may be able to add more colour to that.

It's one of those things that is on the Big List of Things To Change.

It's a sentinel value for the reason described above. It could be any non-mbuf value - Van Jacobsen's birthday if you like.

[gallatin]

For what it is, Van's birthday might be a better choice. Eg, some non-valid pointer that would cause free or mfree to panic rather than accept it and corrupt data.