mbuf: Add a flag for M_HASHTYPE_ to indicate the type has hash properties
ClosedPublic
Actions

Authored by sepherosa_gmail.com on May 17 2016, 3:14 AM.

Details

Reviewers

gallatin
• hselasky
delphij
rrs
glebius
gnn
bz
adrian
rwatson

Group Reviewers

network

Commits

rS300982: mbuf: Add a flag for M_HASHTYPE_ to indicate the type has hash properties

Summary

This flag has not been used, and drivers setting M_HASHTYPE_OPAQUE have not been converted as of this commit.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

sepherosa_gmail.com updated this revision to Diff 16433.May 17 2016, 3:14 AM

sepherosa_gmail.com retitled this revision from to mbuf: Add a flag for M_HASHTYPE_ to indicate the type has hash properties.

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

sepherosa_gmail.com added reviewers: network, adrian, delphij, rwatson, gnn, glebius, rrs, gallatin, bz.May 17 2016, 3:29 AM

sepherosa_gmail.com added a subscriber: freebsd-net-list.

What is the advantage of this bit versus a hash-type range check?

So the real goal here is to be able to express to the LRO code that you have a hash that is usable as a hash (eg, not just an ordinal queue index), and yet warn the RSS code that it is not a toeplitz rss hash result. Is that correct?

In D6406#136051, @gallatin wrote:

So the real goal here is to be able to express to the LRO code that you have a hash that is usable as a hash (eg, not just an ordinal queue index), and yet warn the RSS code that it is not a toeplitz rss hash result. Is that correct?

Yeah

In D6406#136037, @hselasky wrote:

What is the advantage of this bit versus a hash-type range check?

Range check is not helpful. Some drivers' OPAQUE flowid is actually RSS hash value (it just does not know the RSS hash type for some reasons).

If no objection comes, it will be committed tomorrow.

Don't forget to bump the FreeBSD_version before you commit in sys/param.h I think.

sys/sys/mbuf.h
352 ↗	(On Diff #16433)	So IHASH means more than a TOEPLITZ hash?

sepherosa_gmail.com added inline comments.May 24 2016, 5:42 AM

sys/sys/mbuf.h
352 ↗	(On Diff #16433)	Yep, it means the flowid has hash property, e.g. not RX ring index.

• hselasky added inline comments.May 24 2016, 5:53 AM

sys/sys/mbuf.h
352 ↗	(On Diff #16433)	While at it change "((m))" into "(m)". You don't need the double (('s. I wonder if it is better to implement like this, and call it HASHSUBTYPE. That way the schema can be extended in the future. Dividing the numberspace in two by use of a bit is possible a bit overkill. Will there ever by 127 hash types? #define M_HASHSUBTYPE_SET(x) (((x) << 6) & 0xc0) #define M_HASHSUBTYPE_GET(x) (((x) >> 6) & 3) #define M_HASHSUBTYPE_NONE 0 #define M_HASHSUBTYPE_HASHPROP 1 #define M_HASHSUBTYPE_RESERVED_2 2 #define M_HASHSUBTYPE_RESERVED_3 3

sepherosa_gmail.com added inline comments.May 24 2016, 6:00 AM

sys/sys/mbuf.h
352 ↗	(On Diff #16433)	OK, I normally follow "put () around macro parameters" blindly :) It's not a subtype, it just means the the flowid has hash property, e.g. toeplitz hash has property, and other 'sub'-types of packet hash can also have hash property. I don't think there will ever be 127 hash types :P

• hselasky added inline comments.May 24 2016, 6:13 AM

sys/sys/mbuf.h
324 ↗	(On Diff #16433)	Is the phrase "hash property" defined somewhere? Is it better to say "random distribution" property or "cryptographic" ?
352 ↗	(On Diff #16433)	In this case the ()'s are superfluous and can be removed. Why limit the implementation to support only one property?

• hselasky added inline comments.May 24 2016, 6:17 AM

sys/sys/mbuf.h
345 ↗	(On Diff #16433)	Do you plan to convert drivers which use M_HASHTYPE_OPAQUE_HASH and simply pass the ring number as flowid. What hashtype should these drivers use after your changes?

sepherosa_gmail.com added inline comments.May 24 2016, 6:38 AM

sys/sys/mbuf.h
324 ↗	(On Diff #16433)	I just read a lot of "properties" in hash function definition. So I think "hash properties" could fit here. I am not a native speaker, so please advise :) Random distribution sounds good. But I don't think its related to cryptographic.
345 ↗	(On Diff #16433)	The plan is: If the flowid is set to the ring index, we don't change the driver at all. If the flowid is set to some hardware provided hash (normally its toeplitz hash), we change the hashtype to OPAQUE_HASH. Current known drivers need modification is cxgb, mlx5 and hn.
352 ↗	(On Diff #16433)	I am not limiting it, but 8 bits hash type is the constraint here. I _think_ 1 more bit could be reserved. IMHO, we can just wait for the next time when the additional hash type property is needed. However, we can make the OPAQUE 63 instead of 127.

sepherosa_gmail.com added a reviewer: • hselasky.May 24 2016, 6:38 AM

sepherosa_gmail.com removed a subscriber: • hselasky.

• hselasky added inline comments.May 24 2016, 6:54 AM

sys/sys/mbuf.h
345 ↗	(On Diff #16433)	Hi, The current logic is such that you need to set hashtype in order for the flowid to be used, so If the flowid is set to the ring index, we don't change the driver at all. These drivers currently use OPAQUE_HASH ? If the flowid is set to some hardware provided hash These drivers also use OPAQUE_HASH ? I don't understand? Where is the first OPAQUE_HASH type defined which doesn't have the HASHPROP bit set?

sepherosa_gmail.com added inline comments.May 24 2016, 7:06 AM

sys/sys/mbuf.h
345 ↗	(On Diff #16433)	No, only OPAQUE is defined and used currently (OPAQUE_HASH does not exist in the current code base). I will change cxgb, mlx5 and hn to use OPQAQUE_HASH, once this patch is in.