Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 78510 Build 75393: arc lint + arc unit
Event Timeline
The one thing I don't understand is the duality between M_HASHTYPE_OPAQUE and M_HASHTYPE_OPAQUE_HASH. I removed this duality as I don't see any sense in it.
There is no summary or proposed commit message so the motivation for this change is unclear.
Yes, shared that as a preview rather than something 100% ready for commit.
The motivation is a bug fixed by b06db8c9dafb1734923e0483323920b4b0a05cc9 and the test added in it. The fact hash types are distinct values (not flags to be combined) should be better expressed by the declarations rather covered by tests.
this may go back to flow director stuff way back when.
OPAQUE_HASH is that it's a hash that isn't necessarily RSS but has hash properties. Ie it's not some small numerical space of possible values. I think OPAQUE was used by flow director to be able to direct /very specific flows/ in specific ways, rather than how RSS determines how to distribute traffic based on hash.
Oh, and something else - I've started talking with Nick about what it would take to start programming the L4 tables in NICs (starting with ixgbe, but lots of other hardware now supports this) to be able to peel specific 4-tuples off to specific tx/rx rings. That way we can do things like peel a specific socket or set of sockets to a dedicated TX/RX ring. Linux does this with io_uring and eBPF rules. This is the kind of thing that flow director used to support.
What exactly are "hash properties"? With the current use M_HASHTYPE_HASHPROP seems to be single bit that marks rsstype as set to some value.
The OPAQUE seems to look like a mask, but grepping across src/sys doesn't show any real use as a mask.
I would err toward leave it separate (Chesterton's Fence) without ack from existing driver authors like @kib, M_HASHTYPE_OPAQUE describes an RX queue or other ordering, while M_HASHTYPE_OPAQUE_HASH describes a (non-Topliz?) hash. mlx5en uses both. Perhaps someone has some reason to experiment with that, and they manifest differently under M_HASHTYPE_ISHASH. M_HASHTYPE_ISHASH affects at least vxlan.
I'm fine with the general premise aside. This would be tricky to MFC, particularly for external drivers. You can remove the test in this diff, or maybe it could be covered adequately by a handful of static asserts?
Does m_adj_decap() use rssflags &= M_HASHFLAG_INNER, which retains the inner flag where it should clear it?
In general, I'm not sure I like this change. The old code seems less complex once the difference between M_HASHTYPE_OPAQUE and M_HASHTYPE_OPAQUE_HASH is taken into account.
| sys/sys/mbuf.h | ||
|---|---|---|
| 181 | This needs to be separate. M_HASHTYPE_OPAQUE usually means queue index, whereas M_HASHTYPE_OPAQUE_HASH is some non-standard hash result. | |
| 574 | You'd also have to check that its !- to M_HASHTYPE_OPAQUE | |
Why? The old code needed to filter hash type with a mask. The new code reduces all the macros to assignments and comparisons and provides a type. The old code allowed to plant a bug like the one fixed today. The new code, being strengthened with -Werror=assign-enum, such bug can not longer be compilable.
| sys/sys/mbuf.h | ||
|---|---|---|
| 574 | M_HASHTYPE_GET would seem marginally cleaner than deref as the line above | |
tests/sys/sys/mbuf_test.c still refs M_HASHTYPE_INNER so it needs to be updated or removed
M_HASHTYPE_SET and M_HASHTYPE_CLEAR no longer reset INNER is that an issue?
Just an informational note.. the renumbering affects sockopt, netstat output, siftr, dtrace so it cannot MFC.
if_rxd_info grows from 56 to 64b, probably benign.
| sys/sys/mbuf.h | ||
|---|---|---|
| 576 | I think this will break userland build, needs stdbool.h? | |
IMHO, the test is not needed. I will include removal of the test into the change.
M_HASHTYPE_SET and M_HASHTYPE_CLEAR no longer reset INNER is that an issue?
Just an informational note.. the renumbering affects sockopt, netstat output, siftr, dtrace so it cannot MFC.
I don't think this needs to be MFC-ed at all. But theoretically it could, since IP_FLOWTYPE isn't documented and netstat just prints a binary value. Do you know of any practical use of IP_FLOWTYPE?
if_rxd_info grows from 56 to 64b, probably benign.
This can be addressed with bit field specification. Let me know if needs to be done so, or better leave as is?
The old code just makes more sense to me. I agree that yours is technically "better", but this just seems more natural the way it was. No objection to committing it as long as you make sure the default size of enum doesn't bloat things.