extend the use of the th_flags accessor function

[rscheff]

Summary

Formally, there are 12 bits for TCP header flags.
Use the accessor functions in more (kernel) places.

No functional change.

[cy]

This fails to build in /sbin/natd.

ld: error: undefined reference due to --no-allow-shlib-undefined: tcp_get_flags

referenced by /export/obj/opt/src/git-src/amd64.amd64/tmp/usr/lib/libalias.so

cc: error: linker command failed with exit code 1 (use -v to see invocation)

• Error code 1

Stop.
make[4]: stopped making "all" in /opt/src/git-src/sbin/natd
.ERROR_TARGET='natd.full'

[rscheff]

• use userspace accessor function

[kp]

I'm happy with the pf bits.
It mostly doesn't matter because we tend to only look for th_flags flags and not th_x2, but it's probably better to use the accessor anyway.

We should probably do this too:

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 30be1128d4d3..88f9c236b14d 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -2338,7 +2338,7 @@ extern struct pf_ksrc_node        *pf_find_src_node(struct pf_addr *,
 extern void                     pf_unlink_src_node(struct pf_ksrc_node *);
 extern u_int                    pf_free_src_nodes(struct pf_ksrc_node_list *);
 extern void                     pf_print_state(struct pf_kstate *);
-extern void                     pf_print_flags(u_int8_t);
+extern void                     pf_print_flags(uint16_t);
 extern int                      pf_addr_wrap_neq(struct pf_addr_wrap *,
                                    struct pf_addr_wrap *);
 extern u_int16_t                pf_cksum_fixup(u_int16_t, u_int16_t, u_int16_t,
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index bd8b709e396e..47134a169a68 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -2891,7 +2891,7 @@ pf_print_state_parts(struct pf_kstate *s,
 }

 void
-pf_print_flags(u_int8_t f)
+pf_print_flags(uint16_t f)
 {
        if (f)
                printf(" ");
@@ -2911,6 +2911,8 @@ pf_print_flags(u_int8_t f)
                printf("E");
        if (f & TH_CWR)
                printf("W");
+       if (f & TH_AE)
+               printf("[AE]");
 }

 #define        PF_SET_SKIP_STEPS(i)                                    \

[kp]

sys/netinet/tcp_output.c

1311

This looks weird though. Do we really want to not update the TCP header with this flag?
I may have missed it because it's a fairly large function, but it doesn't look like we end up applying 'flags' to 'th_flags' later.

[rscheff]

• add TH_AE decoding (identical to tcpdump as "e") as suggested by kp
• moving updating the th_flags further down past the last adjustment

[rscheff]

In D47063#1073100, @kp wrote:

I'm happy with the pf bits.
It mostly doesn't matter because we tend to only look for th_flags flags and not th_x2, but it's probably better to use the accessor anyway.

We should probably do this too
...

Thanks. I added this, but used the same character ("e") used by tcpdump ( https://github.com/the-tcpdump-group/tcpdump/pull/999/commits/55dc3206aa07c3cdfe19b8d23e7b134d37b9a6b7#diff-7334e7c33e9abb09f57bd40e3c24fbb29a7088bf0ab7cc2f499b1425fc6b9741 ) for consistency (even though here we use "A" instead of "." for the ACK flag).

sys/netinet/tcp_output.c

1311

Thanks for spotting this - I missed that the update to th_flags was above this line - apparently I missed this final th_flags update in the original change. Moved the final back-assignment after this conditional.

[rscheff]

• extend the use of accessor function in userspace and pfil

[kp]

In D47063#1073412, @rscheff wrote:

Thanks. I added this, but used the same character ("e") used by tcpdump

Yeah, that's better.

I'm still happy with the pf bits.

[cc]

I current concern is that new code for the TH_AE shall be in a separate patch, so that this patch can be a pure big non-functional change.

sys/netinet/libalias/alias.c

186

If possible, suggest to use the explicit uint16_t for the type of th_flags throughout this change.

sys/netpfil/ipfilter/netinet/ip_compat.h

1131–1135

Suggest to separate this new code into a new patch.

sys/netpfil/pf/pf.c

2914

Same here, new code shall be in new patch.

usr.sbin/ppp/ip.c

374–376 ↗

(On Diff #144782)

For readability, can we also make it cleaner by removing the outer ()? For example:

estab = __tcp_get_flags(th) & TH_ACK;

564 ↗

(On Diff #144782)

Same here, new code shall be in a new patch.

[rscheff]

• clean up type and remove superfluous parenthesis
• revert adding support for TH_AE in ppp and pf

[rscheff]

• missed function definition

[cc]

sys/netpfil/ipfw/ip_fw2.c

1024 ↗

(On Diff #144822)

Remove the extra space in if ( (.

[cy]

Builds now. Thanks.

[rscheff]

• use accessor in various drivers

[jhb]

This seems fine to me (cxgbe bits). In some places I misread tcp_set_flags as being an or (I) operation rather than assignment (=) since in English prose "setting a flag" usually means enabling a flag (or flags) leaving other bits unchanged. In a similar vein, having a tcp_clear_flags helper that took a mask of bits to clear might be nicer to replace places that previously used &= ~. Some sort of wrapper for |= might also be nice. I'm not sure what to call that wrapper though without further tripping over the ambiguity of tcp_set_flags. (atomic_set_* set bits rather than assigning values and that has caused some confusion in the past as well FWIW.)

[rscheff]

@kbowling, can you have a look at the intel driver changes and if they seem ok, remove that blocker?

[kbowling]

sys/net/iflib.h

128 ↗

(On Diff #145825)

I think you are filling a hole but this is used in the hot path to check if a packet has a vlan tag. I'm not sure offhand if moving this has any positive or negative impact on performance but it would almost always be accessed near the other fields where it used to be, so if it fit in the same cacheline or prefetch it could be relevant to leave it. Something to ponder for a moment unless you already have.

ipi_tcp_flags and ipi_tcp_seq would be cold, ipi_ip_tos is lukewarm (one TX consumer).

[rscheff]

sys/net/iflib.h

128 ↗

(On Diff #145825)

Thanks for the heads up. I double checked - the entire struct is 48 bytes, which fits into a single cacheline on all current platforms. On older pentiums with 32 byte cachelines, the members up to ipi_vtag would be in the 1st (moving from 64bit pointer to 32bit), and everything after in the 2nd. Shifting ipi_mflags around like this remains in the same cacheline with ipi_etype, ipi_tcp_hflags, ipi_tcp_seq and ipi_ip_tos. Or with other words - regardless of achitecture and 32 or 64 bit cachelines, the new location retains the very same cacheline locality as before.

if_pkt_info	
0	*ipi_segs
.	
.	
.	
.	
.	
.	
.	
8	ipi_len
.	
.	
.	
12	ipi_qsidx
.	
14	ipi_nsegs
.	
16	ipi_ndescs
.	
18	ipi_flags
.	
20	ipi_pidx
.	
22	ipi_new_pidx
.	
24	ipi_ehdrlen
25	ipi_ip_hlen
26	ipi_tcp_hlen
27	ipi_ipproto
28	ipi_csum_flags
.	
.	
.	
32	ipi_tso_segsz
.	
34	ipi_vtag
.	
36	ipi_etype
.	
38	ipi_tcp_hflags
.	
40	ipi_tcp_seq
.	
.	
.	
44	ipi_ip_tos
45	ipi_mflags
46	spare0
47	spare1