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

[sepherosa_gmail.com]

Summary

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

[• hselasky]

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

[gallatin]

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?

[sepherosa_gmail.com]

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

[sepherosa_gmail.com]

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

[sepherosa_gmail.com]

If no objection comes, it will be committed tomorrow.

[• hselasky]

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

sys/sys/mbuf.h

352

So IHASH means more than a TOEPLITZ hash?

[sepherosa_gmail.com]

sys/sys/mbuf.h

352

Yep, it means the flowid has hash property, e.g. not RX ring index.

[• hselasky]

sys/sys/mbuf.h

352

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]

sys/sys/mbuf.h

352

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]

sys/sys/mbuf.h

324

Is the phrase "hash property" defined somewhere?

Is it better to say "random distribution" property or "cryptographic" ?

352

In this case the ()'s are superfluous and can be removed.

Why limit the implementation to support only one property?

[• hselasky]

sys/sys/mbuf.h

345

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]

sys/sys/mbuf.h

324

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

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

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.

[• hselasky]

sys/sys/mbuf.h

345

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]

sys/sys/mbuf.h

345

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.

[• hselasky]

sys/sys/mbuf.h

345

OK, that makes sense.

Let's wait and see if more people have any comments.

Did you fix the extra ()'s ?

--HPS

[sepherosa_gmail.com]

sys/sys/mbuf.h

345

I will update the patch later today. Cleaning up the vmbus now :P

[sepherosa_gmail.com]

Address hps' comment