In D46881#1069245, @markj wrote:

All of the races are harmless though, i.e., the getsockopt call would return garbage in the worst case. There might be applications which query these socket options frequently, especially the socket buffer sizes. Do you think the extra overhead of acquiring the socket lock is unlikely to matter?

It would be great if you can do a functional test...

In D46824#1068981, @jhb wrote:

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

In D46824#1067840, @rscheff wrote:

Good find! While there is probably zero difference as the compiler optimizes this, keeping "old_len" instead would only require the clearing of TH_FIN, and not another assignment though, which seems more alike to similar other checks... @tuexen @cc and @rrs may want to have a look too...

In D46779#1066415, @glebius wrote:

Internally at Netflix I will make all private options relative to TCP_VENDOR. This will make my merging much easier. Ideally we even want this to become a enum, and that would protect again any collisions and other failures.

This was committed in cbc9438f0505bd971e9eba635afdae38a267d76e.

In D46793#1067203, @markj wrote:

In D46793#1067201, @tuexen wrote:

In D46793#1067164, @markj wrote:

This looks right to me, but I'm not an expert on this code.

Probably the MAC label allocation can also be deferred this way?

The story is different for the MAC label, since the MAC label allocation can result in an error and we need a MAC label to send a response. Of course, you can delay the allocation of the MAC label, but then you have to undo things you allocated before. Right now the sequence is MAC label first, ipopts next, and syncache entry last... If you prefer, we can change that. But my goal was to fix the leaks...

Ok, fair enough. It was just a question, I already clicked the magic "accept" button.

In D46793#1067164, @markj wrote:

This looks right to me, but I'm not an expert on this code.

Probably the MAC label allocation can also be deferred this way?

In D44258#1067037, @erj wrote:

This means that the PSH and FIN flag correctly appear only in the last segment, but the CWR flag does not only appear in the first segment, but also in all middle segments.
Using this patch, this could be fixed.

When doing Accurate ECN as currently being specified in Accurate ECN, the masks should be:

mask.first	0xFF6
mask.middle	0xFF6
mask.last	0xFFF

The proposed patch would allow a NIC to be configured to do TSO for either classical ECN or accurate ECN.

Does this description make the motivation clear?

Your description definitely helps; thanks for asking the question @kbowling. @tuexen have you checked these masks on the other devices that use ixl/ice, or is this what you've observed so far?

In D44258#1067035, @kbowling wrote:

In D44258#1067031, @tuexen wrote:

In D44258#1066453, @kbowling wrote:

In D44258#1066450, @tuexen wrote:

In D44258#1066441, @kbowling wrote:

Are you sure this can't be dealt with dynamically in ix_txrx? Admittedly I have no reason to spend a lot of time digging into this but my intuition is you can stuff the offsets into if_pkt_info_t in iflib.c and make the right decisions when constructing the TSO packet descriptor.

Are you saying you would write the register with every TSO packet? I have no experience, but I thought this might be too expensive. Right now, the NIC is using one behavior for all TSO packets for all connections...

No, I was speculating maybe you can accomplish whatever header change is desired in the TSO descriptor rather than globally altering those registers. But I do not know.

I believe by default whatever is in the pseudoheader is copied to all segments.

What are we trying to accomplish.. is a default mask ruining the ECN flag or are you trying to drop/alter the ECN flag in the middles or last segment?

In D44258#1066453, @kbowling wrote:

In D44258#1066450, @tuexen wrote:

In D44258#1066441, @kbowling wrote:

Are you sure this can't be dealt with dynamically in ix_txrx? Admittedly I have no reason to spend a lot of time digging into this but my intuition is you can stuff the offsets into if_pkt_info_t in iflib.c and make the right decisions when constructing the TSO packet descriptor.

Are you saying you would write the register with every TSO packet? I have no experience, but I thought this might be too expensive. Right now, the NIC is using one behavior for all TSO packets for all connections...

No, I was speculating maybe you can accomplish whatever header change is desired in the TSO descriptor rather than globally altering those registers. But I do not know.

I believe by default whatever is in the pseudoheader is copied to all segments.

What are we trying to accomplish.. is a default mask ruining the ECN flag or are you trying to drop/alter the ECN flag in the middles or last segment?

Here is what I'm trying to accomplish:
The TCP header contains 12 flags (see IANA. The Intel NIC uses three 12 bit masks to compute, which flags are copied to the first, the middle, and the last segment when performing TSO. When doing ECN as specified in RFC 3168, the masks should be:

mask.first	0xFF6
mask.middle	0xF76
mask.last	0xF7F

This means that the FIN and PSH flag only appear in the last segment and the CWR flag only appear in the first segment. Several Intel NICs behave like this.
However,

ix0: <Intel(R) X520 82599ES (SFI/SFP+)> mem 0x6004000000000-0x600400007ffff,0x6004000100000-0x6004000103fff irq 1040376 at device 0.0 numa-domain 0 on pci3
ix0: Using 2048 TX descriptors and 2048 RX descriptors
ix0: Using 16 RX queues 16 TX queues
ix0: Using MSI-X interrupts with 17 vectors
ix0: allocated for 16 queues
ix0: allocated for 16 rx queues
ix0: Ethernet address: 90:e2:ba:f7:48:74
ix0: PCI Express Bus: Speed 5.0GT/s Width x8
ix0: eTrack 0x000161c1

reports

dev.ix.0.tso_tcp_flags_mask_first_segment: 0x00000ff6
dev.ix.0.tso_tcp_flags_mask_middle_segment: 0x00000ff6
dev.ix.0.tso_tcp_flags_mask_last_segment: 0x00000f7f

This means that the PSH and FIN flag correctly appear only in the last segment, but the CWR flag does not only appear in the first segment, but also in all middle segments.
Using this patch, this could be fixed.

When doing Accurate ECN as currently being specified in Accurate ECN, the masks should be:

mask.first	0xFF6
mask.middle	0xFF6
mask.last	0xFFF

The proposed patch would allow a NIC to be configured to do TSO for either classical ECN or accurate ECN.

Does this description make the motivation clear?

It does, thank you. There is nothing wrong with the patch as is. Should we program the registers automatically for normal ECN in a followup as the newer NICs do? I am guessing they are the way they are due to the age of the hardware/driver.

In D44258#1066453, @kbowling wrote:

In D44258#1066450, @tuexen wrote:

In D44258#1066441, @kbowling wrote:

Are you sure this can't be dealt with dynamically in ix_txrx? Admittedly I have no reason to spend a lot of time digging into this but my intuition is you can stuff the offsets into if_pkt_info_t in iflib.c and make the right decisions when constructing the TSO packet descriptor.

Are you saying you would write the register with every TSO packet? I have no experience, but I thought this might be too expensive. Right now, the NIC is using one behavior for all TSO packets for all connections...

No, I was speculating maybe you can accomplish whatever header change is desired in the TSO descriptor rather than globally altering those registers. But I do not know.

I believe by default whatever is in the pseudoheader is copied to all segments.

What are we trying to accomplish.. is a default mask ruining the ECN flag or are you trying to drop/alter the ECN flag in the middles or last segment?

Only get a reference counter to so->so_cred when it is actually needed as suggested by Mark.

In D44258#1066441, @kbowling wrote:

Are you sure this can't be dealt with dynamically in ix_txrx? Admittedly I have no reason to spend a lot of time digging into this but my intuition is you can stuff the offsets into if_pkt_info_t in iflib.c and make the right decisions when constructing the TSO packet descriptor.

I was using the style used in netinet/in.h. But I do not care how we mark them as unused as long as it is consistent.

Initialize maclabel to NULL, which is not need right now, but avoids using an uninitialized variable in case someone adds a goto in the future before mac_syncache_init() is called. This was suggested by Mark.

Remove parentheses, which are not necessary, as suggested by Mark.

Fix mac_getsockopt_label(), not mac_setsockopt_label() as pointed out by Mark.

Address Mark's comment.

In D46669#1063601, @np wrote:

I'd be okay with removing the condition entirely. It is useful to have core data structures be a fixed size independent of configuration.

In D46428#1060788, @cc wrote:

Besides, I am wondering if TCPSTAT_INC(tcps_sndacks) and TCPSTAT_INC(tcps_sndtotal) consistency can also be improved after successful syncache_respond().

Improve consistency of the TCP stats counters as suggested by cc@.

This was committed in b2044c4557443bbce974101f04e2b465d1bbe769.

In D46437#1058468, @cc wrote:

Are you planning to remove the corresponding code in kernel space in a separate patch?