Page MenuHomeFreeBSD

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

Authored by shurd on Aug 24 2018, 3:38 PM.
Tags
None
Referenced Files
F105962852: D16882.diff
Mon, Dec 23, 4:21 AM
Unknown Object (File)
Fri, Dec 20, 10:58 AM
Unknown Object (File)
Fri, Dec 20, 10:32 AM
Unknown Object (File)
Fri, Dec 20, 10:29 AM
Unknown Object (File)
Wed, Dec 18, 11:19 AM
Unknown Object (File)
Nov 21 2024, 7:27 PM
Unknown Object (File)
Nov 21 2024, 7:27 PM
Unknown Object (File)
Nov 21 2024, 7:26 PM
Subscribers

Details

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().

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19243
Build 18856: arc lint + arc unit

Event Timeline

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

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

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

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.

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...

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.

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.

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.

This revision is now accepted and ready to land.Aug 29 2018, 10:08 AM
This revision was automatically updated to reflect the committed changes.