Add a switch structure for send tags.

[jhb]

Summary

Move the function pointers for operations on existing send tags
(modify, query, next, free) out of 'struct ifnet' and into a new
'struct if_snd_tag_sw'. A pointer to this structure is added to the
generic part of send tags and is initialized by m_snd_tag_init()
(which now accepts a a switch structure as a new argument).

Previously, device driver ifnet methods switched on the type to call
type-specific functions. Now, those type-specific functions are saved
in the switch structure and invoked directly. In addition, this more
gracefully permits multiple implementations of the same tag within a
driver. In particular, NIC TLS for future Chelsio adapters will use a
different implementation than the existing NIC TLS support for T6
adapters.

Sponsored by: Chelsio Communications

[jhb]

I've run-tested this with NIC TLS on T6, and it compiles for mlx5.

This version of the patch is kind of "least invasive". Arguably, we could move the new structure and the associated function pointer typedefs to <sys/mbuf.h> and use 'm_*' instead of 'if_*' such as 'struct m_snd_tag_sw' for the new structure.

The other consideration is that the send tag API might get reused for NIC KTLS RX and it might be worth renaming these to be more like 'm_if_tag' and 'm_if_tag_sw' or something along those lines. I kind of think that sort of mass rename probably should be in a separate commit as it will add much more churn.

[gallatin]

I like this.

[• hselasky]

Looks good. I will review this more closely next week.

[• hselasky]

Hi @jhb

Another parameter which is constant is the "mst->type". Should this be encoded into the "sw" structure aswell?

	mst->type = type; // Do we really need this here? Save some memory?
	mst->sw = sw;

        mst->sw->type ??

[• hselasky]

And then we can add a special mbuf send tag type for proxying layers like lagg?

[jhb]

In D31572#713607, @hselasky wrote:

Hi @jhb

Another parameter which is constant is the "mst->type". Should this be encoded into the "sw" structure aswell?

	mst->type = type; // Do we really need this here? Save some memory?
	mst->sw = sw;

        mst->sw->type ??

Well, due to the ref_cnt, we already have a u_int next to it so if we removed the type it would just leave a padding hole and not actually save memory. Otherwise I probably would have moved it.

[jhb]

In D31572#713610, @hselasky wrote:

And then we can add a special mbuf send tag type for proxying layers like lagg?

We need to preserve the type even in the proxying case so that the TLS + rate limit code to add rate limit still see a "TLS" tag from lagg(4) when deciding whether or not to allocate a new TLS + rate limit tag vs modifying the rate on an existing tag.

[• hselasky]

Looks good to me. Adding Konstantin just in case.

[kib]

In D31572#714267, @jhb wrote:

In D31572#713607, @hselasky wrote:

Hi @jhb

Another parameter which is constant is the "mst->type". Should this be encoded into the "sw" structure aswell?

	mst->type = type; // Do we really need this here? Save some memory?
	mst->sw = sw;

        mst->sw->type ??

Well, due to the ref_cnt, we already have a u_int next to it so if we removed the type it would just leave a padding hole and not actually save memory. Otherwise I probably would have moved it.

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

353

I would put a comma at the end of line

[jhb]

In D31572#716152, @kib wrote:

In D31572#714267, @jhb wrote:

In D31572#713607, @hselasky wrote:

Hi @jhb

Another parameter which is constant is the "mst->type". Should this be encoded into the "sw" structure aswell?

	mst->type = type; // Do we really need this here? Save some memory?
	mst->sw = sw;

        mst->sw->type ??

Well, due to the ref_cnt, we already have a u_int next to it so if we removed the type it would just leave a padding hole and not actually save memory. Otherwise I probably would have moved it.

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

[kib]

In D31572#716970, @jhb wrote:

In D31572#716152, @kib wrote:

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

What do you mean? Is it just one switch structure for vlan, and one for lagg, or something more involved? If just two additional tag switch structures, why is this a problem?

[jhb]

In D31572#717054, @kib wrote:

In D31572#716970, @jhb wrote:

In D31572#716152, @kib wrote:

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

What do you mean? Is it just one switch structure for vlan, and one for lagg, or something more involved? If just two additional tag switch structures, why is this a problem?

It just means currently going from 1 to 4 structures for vlan and lagg (for unlimited, rate limit, TLS, and TLS + rate limit) plus additional tag switches in the future if we add new tag types in the future.

[kib]

In D31572#719197, @jhb wrote:

In D31572#717054, @kib wrote:

In D31572#716970, @jhb wrote:

In D31572#716152, @kib wrote:

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

What do you mean? Is it just one switch structure for vlan, and one for lagg, or something more involved? If just two additional tag switch structures, why is this a problem?

It just means currently going from 1 to 4 structures for vlan and lagg (for unlimited, rate limit, TLS, and TLS + rate limit) plus additional tag switches in the future if we add new tag types in the future.

I do not insist, of course.

[jhb]

In D31572#719475, @kib wrote:

In D31572#719197, @jhb wrote:

In D31572#717054, @kib wrote:

In D31572#716970, @jhb wrote:

In D31572#716152, @kib wrote:

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

What do you mean? Is it just one switch structure for vlan, and one for lagg, or something more involved? If just two additional tag switch structures, why is this a problem?

It just means currently going from 1 to 4 structures for vlan and lagg (for unlimited, rate limit, TLS, and TLS + rate limit) plus additional tag switches in the future if we add new tag types in the future.

I do not insist, of course.

I do appreciate the point as having the type in the structure is more consistent with, e.g. cdevsw.

Here's a commit that moves it to compare both versions (I was already working on this before your last reply): https://github.com/bsdjhb/freebsd/commit/0710302136cdef922c216b777ac60736803a514a

[kib]

In D31572#719596, @jhb wrote:

In D31572#719475, @kib wrote:

In D31572#719197, @jhb wrote:

In D31572#717054, @kib wrote:

In D31572#716970, @jhb wrote:

In D31572#716152, @kib wrote:

It still saves space on 32bit arches. My opinion is same as Hans', this structure should be normalized and type go into tag switch, but I do not insist.

Hmm, so I started on this, but one of the issues is that the tag switches for if_vlan and if_lagg would need to be duplicated for each tag type. In the current patch there is a single one that is able to pass through arbitrary tag types to the underlying interface.

What do you mean? Is it just one switch structure for vlan, and one for lagg, or something more involved? If just two additional tag switch structures, why is this a problem?

It just means currently going from 1 to 4 structures for vlan and lagg (for unlimited, rate limit, TLS, and TLS + rate limit) plus additional tag switches in the future if we add new tag types in the future.

I do not insist, of course.

I do appreciate the point as having the type in the structure is more consistent with, e.g. cdevsw.

Here's a commit that moves it to compare both versions (I was already working on this before your last reply): https://github.com/bsdjhb/freebsd/commit/0710302136cdef922c216b777ac60736803a514a

I do not see anything inherently awful with this patch.

[• hselasky]

I do appreciate the point as having the type in the structure is more consistent with, e.g. cdevsw.

Here's a commit that moves it to compare both versions (I was already working on this before your last reply): https://github.com/bsdjhb/freebsd/commit/0710302136cdef922c216b777ac60736803a514a

I do not see anything inherently awful with this patch.

+1

[jhb]

Not sure why this didn't auto close. I committed a version that folded in the changes to move 'type' into the switch structure.