Add support for rate limited TLS send tags

[• hselasky]

Summary

Introduce a new send tag type designated for ratelimited TLS connections. An own tag type has been chosen to avoid packets being sent out of order when switching on and off ratelimit.

MFC after: 1 week
Sponsored by: Mellanox Technologies

[kib]

sys/net/if_var.h

233

Why are you reusing existing structure ?

[• hselasky]

sys/net/if_var.h

233

Because the arguments are the same.

[kib]

sys/net/if_var.h

233

So you are using max_rate in case of SND_TAG_TYPE_TLS ? If not, I think it is a better interface to add another structure that would contain snd_tag_alloc_tls, for tls_rate_limit.

Or, you may leave only one member of the union with type if_snd_tag_alloc_tls.

[jhb]

How does changing the rate work? Right now you'd have to have all the places that change the rate figure out what type the tag is to send a different structure down for modifying the rate. That is sub-optimal. This also doesn't address the issue of how to make the socket option adjust the rate of an existing tag. Just adding a new tag type is the easy part of the problem, but not a full solution on its own.

As I said previously, I think the right fix is to rework how send tag modifies (and queries work) to take an explicit tag that is a "feature" and permit send tags to implement multiple features. Chelsio's hardware can support rate limits on existing TLS tags without requiring a dedicated type as well and teaching modify/query to support an explicit feature instead of an implicit one will permit Chelsio's bits to allow setting the rate on existing connections without requiring a new tag whereas for Mellanox the code will have to fallback to allocating a new combo tag if the modify fails.

I also think that a separate structure is a better approach. Probably the existing union needs to be replaced with something like this:

struct if_snd_tag_alloc_params {
    uint32_t type;
    uint32_t flowid;
    uint32_t flowtype;
    union  {
         struct if_snd_tag_alloc_ratelimit ratelimit;
         struct if_snd_tag_alloc_tls tls;
         struct {
                    struct if_snd_tag_ratelimit ratelimit;
                    struct if_snd_tag_alloc_tls tls
         } tls_ratelimit;
    }
};

This would require removing the hdr member from the if_snd_tag_alloc structures. Even nicer would be if we could reuse the same structures for modify, query (which nothing uses anyway), and the union in alloc.

[• hselasky]

In D22669#495980, @jhb wrote:

How does changing the rate work? Right now you'd have to have all the places that change the rate figure out what type the tag is to send a different structure down for modifying the rate. That is sub-optimal. This also doesn't address the issue of how to make the socket option adjust the rate of an existing tag. Just adding a new tag type is the easy part of the problem, but not a full solution on its own.

I suggest that changing the rate is feature of the TAG type, which use a standard ratelimit modify request. Eventually we could have a modify type which tell exactly what to modify so there is no problem with different structures.

I will make a patch for this separately.

I would like to have a separated tag for TLS ratelimit, because it requires an own send queue, and I guess for Chelsio ethernet it will be the same! This way we avoid allocating more resources than needed.

As I said previously, I think the right fix is to rework how send tag modifies (and queries work) to take an explicit tag that is a "feature" and permit send tags to implement multiple features. Chelsio's hardware can support rate limits on existing TLS tags without requiring a dedicated type as well and teaching modify/query to support an explicit feature instead of an implicit one will permit Chelsio's bits to allow setting the rate on existing connections without requiring a new tag whereas for Mellanox the code will have to fallback to allocating a new combo tag if the modify fails.

I also think that a separate structure is a better approach. Probably the existing union needs to be replaced with something like this:

Please suggest this factoring as a separate patch.

struct if_snd_tag_alloc_params {
    uint32_t type;
    uint32_t flowid;
    uint32_t flowtype;
    union  {
         struct if_snd_tag_alloc_ratelimit ratelimit;
         struct if_snd_tag_alloc_tls tls;
         struct {
                    struct if_snd_tag_ratelimit ratelimit;
                    struct if_snd_tag_alloc_tls tls
         } tls_ratelimit;
    }
};

This would require removing the hdr member from the if_snd_tag_alloc structures. Even nicer would be if we could reuse the same structures for modify, query (which nothing uses anyway), and the union in alloc.

Can I move forward with this patch? It is currently blocking upstreaming the Mellanox HW TLS code.

[• hselasky]

sys/net/if_var.h

233

It is a tradeoff. I don't want more structures, but less.

[jhb]

As I have told you multiple times, it is _not_ the same for Chelsio as TLS send tags on Chelsio always support rate limiting. However, it doesn't seem like this is blocking upstreaming since you've just committed the non-ratelimit TLS code today? Given that it's not practical to do the actual rate setting until you refactor, the current patch seems useless without the refactoring, so I think doing the refactor first makes more sense.

[• hselasky]

In D22669#496485, @jhb wrote:

As I have told you multiple times, it is _not_ the same for Chelsio as TLS send tags on Chelsio always support rate limiting. However, it doesn't seem like this is blocking upstreaming since you've just committed the non-ratelimit TLS code today?

No, the ethernet part is still missing and I'm blocked.

Given that it's not practical to do the actual rate setting until you refactor, the current patch seems useless without the refactoring, so I think doing the refactor first makes more sense.

Mellanox NICs will re-order packets when you switch packet pacing on and off on the same tag. Also for TLS. So we would like to have a separate tag for this and not one tag with feature on/off like you suggest.

We don't have a queue per TLS connection unless you use packet pacing, but it appears Chelsio does. I don't understand how Chelsio can disable/enable packet pacing seamlessly? Can you explain how the hardware does that?

[jhb]

Subsumed into D26691.