Page MenuHomeFreeBSD
Differential D43560

tcp: improve tcp_ecn by using switch statements and bit-shifting
ClosedPublic
Actions

Authored by rscheff on Jan 23 2024, 2:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 9, 1:26 AM
Unknown Object (File)
Thu, Mar 6, 5:58 PM
Unknown Object (File)
Jan 29 2025, 11:27 PM
Unknown Object (File)
Jan 28 2025, 4:14 AM
Unknown Object (File)
Jan 24 2025, 5:51 AM
Unknown Object (File)
Jan 17 2025, 8:43 PM
Unknown Object (File)
Jan 5 2025, 11:39 PM
Unknown Object (File)
Nov 21 2024, 6:43 AM
View All 48 Files
Subscribers
imp
melifaro

Details

Reviewers
cc
glebius
tuexen
thj
rrs
Group Reviewers
transport
Commits
rGfc262fd3dc0a: tcp: AccECN access ACE field by shifting bits
Summary

In order to improve readability, use switch statements to
differentiate between the different ECN signallings.

Also, access the ACE field by bit-shifting instead checking
each flag individually.

No change in behaviour.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 55575
Build 52464: arc lint + arc unit

Event Timeline

rscheff created this revision.Jan 23 2024, 2:49 PM
Herald added a reviewer: transport. · View Herald TranscriptJan 23 2024, 2:49 PM
Herald added subscribers: melifaro, imp. · View Herald Transcript
rscheff requested review of this revision.Jan 23 2024, 2:49 PM
Harbormaster completed remote builds in B55569: Diff 133197.Jan 23 2024, 2:49 PM
glebius added a comment.Jan 23 2024, 6:36 PM

P.S. There are so many sysctl toggles that ask for SYSCTL_INT_MINMAX() macro, to avoid coding out yet another value checking function but to block user from entering invalid value.

sys/netinet/tcp_ecn.c
103

AFAIK, these should be one liners per style(9).

123

Not sure it is documented, but usual habit is to leave default: as the last case.

glebius accepted this revision.Jan 23 2024, 6:36 PM
rscheff updated this revision to Diff 133228.Jan 23 2024, 7:17 PM
  • remove default case, as falling through and returning is the correct action.
Harbormaster completed remote builds in B55575: Diff 133228.Jan 23 2024, 7:18 PM
tuexen added inline comments.Jan 24 2024, 7:17 AM
sys/netinet/tcp_ecn.c
226

The indentation looks strange here...

rscheff updated this revision to Diff 133284.Jan 24 2024, 4:26 PM
rscheff marked an inline comment as done.
  • remove default: when no special action is necessary
  • place statemens after case: in new line
  • add /* FALLTHROUGH */ comments per style(9)
Harbormaster completed remote builds in B55594: Diff 133284.Jan 24 2024, 4:26 PM
rscheff marked an inline comment as done.Jan 24 2024, 4:27 PM
tuexen accepted this revision.Jan 25 2024, 8:28 PM
This revision is now accepted and ready to land.Jan 25 2024, 8:28 PM
Closed by commit rGfc262fd3dc0a: tcp: AccECN access ACE field by shifting bits (authored by rscheff). · Explain WhyJan 25 2024, 11:19 PM
This revision was automatically updated to reflect the committed changes.
rscheff added a commit: rGfc262fd3dc0a: tcp: AccECN access ACE field by shifting bits.

Revision Contents
Changeset List

PathSize
sys/
netinet/
3 lines
67 lines

Diff 133228

View Options

sys/netinet/tcp_ecn.h

Show First 20 Lines • Show All 131 Lines • ▼ Show 20 Lines
*/ */
#define IEEE80211_CIPHER_WEP 0 #define IEEE80211_CIPHER_WEP 0
#define IEEE80211_CIPHER_TKIP 1 #define IEEE80211_CIPHER_TKIP 1
#define IEEE80211_CIPHER_AES_OCB 2 #define IEEE80211_CIPHER_AES_OCB 2
#define IEEE80211_CIPHER_AES_CCM 3 #define IEEE80211_CIPHER_AES_CCM 3
#define IEEE80211_CIPHER_TKIPMIC 4 /* TKIP MIC capability */ #define IEEE80211_CIPHER_TKIPMIC 4 /* TKIP MIC capability */
#define IEEE80211_CIPHER_CKIP 5 #define IEEE80211_CIPHER_CKIP 5
#define IEEE80211_CIPHER_NONE 6 /* pseudo value */ #define IEEE80211_CIPHER_NONE 6 /* pseudo value */
#define IEEE80211_CIPHER_AES_CCM_256 7
#define IEEE80211_CIPHER_BIP_CMAC_128 8
#define IEEE80211_CIPHER_BIP_CMAC_256 9
#define IEEE80211_CIPHER_BIP_GMAC_128 10
#define IEEE80211_CIPHER_BIP_GMAC_256 11
#define IEEE80211_CIPHER_AES_GCM_128 12
#define IEEE80211_CIPHER_AES_GCM_256 13
bzUnsubmitted
Not Done
Inline Actions

I love enums.

bz: I love enums.
ccUnsubmitted
Not Done
Inline Actions

I love enums.

+1

cc: > I love enums. +1
adrianAuthorUnsubmitted
Done
Inline Actions

I do too, but they're already defines. I'm happy to add it to a follow-up enum cleanup task?

We just need to be sure there's nothing in the codebase and wpa/hostapd driver_bsd.c code that's using these things as compile time flags?

adrian: I do too, but they're already defines. I'm happy to add it to a follow-up enum cleanup task?
#define IEEE80211_CIPHER_MAX (IEEE80211_CIPHER_NONE+1) #define IEEE80211_CIPHER_LAST 13
#define IEEE80211_CIPHER_MAX (IEEE80211_CIPHER_LAST+1)
/* capability bits in ic_cryptocaps/iv_cryptocaps */ /* capability bits in ic_cryptocaps/iv_cryptocaps */
#define IEEE80211_CRYPTO_WEP (1<<IEEE80211_CIPHER_WEP) #define IEEE80211_CRYPTO_WEP (1<<IEEE80211_CIPHER_WEP)
#define IEEE80211_CRYPTO_TKIP (1<<IEEE80211_CIPHER_TKIP) #define IEEE80211_CRYPTO_TKIP (1<<IEEE80211_CIPHER_TKIP)
#define IEEE80211_CRYPTO_AES_OCB (1<<IEEE80211_CIPHER_AES_OCB) #define IEEE80211_CRYPTO_AES_OCB (1<<IEEE80211_CIPHER_AES_OCB)
#define IEEE80211_CRYPTO_AES_CCM (1<<IEEE80211_CIPHER_AES_CCM) #define IEEE80211_CRYPTO_AES_CCM (1<<IEEE80211_CIPHER_AES_CCM)
#define IEEE80211_CRYPTO_TKIPMIC (1<<IEEE80211_CIPHER_TKIPMIC) #define IEEE80211_CRYPTO_TKIPMIC (1<<IEEE80211_CIPHER_TKIPMIC)
#define IEEE80211_CRYPTO_CKIP (1<<IEEE80211_CIPHER_CKIP) #define IEEE80211_CRYPTO_CKIP (1<<IEEE80211_CIPHER_CKIP)
#define IEEE80211_CRYPTO_AES_CCM_256 (1<<IEEE80211_CIPHER_AES_CCM_256)
#define IEEE80211_CRYPTO_BIP_CMAC_128 (1<<IEEE80211_CIPHER_BIP_CMAC_128)
#define IEEE80211_CRYPTO_BIP_CMAC_256 (1<<IEEE80211_CIPHER_BIP_CMAC_256)
#define IEEE80211_CRYPTO_BIP_GMAC_128 (1<<IEEE80211_CIPHER_BIP_GMAC_128)
#define IEEE80211_CRYPTO_BIP_GMAC_256 (1<<IEEE80211_CIPHER_BIP_GMAC_256)
#define IEEE80211_CRYPTO_AES_GCM_128 (1<<IEEE80211_CIPHER_AES_GCM_128)
#define IEEE80211_CRYPTO_AES_GCM_256 (1<<IEEE80211_CIPHER_AES_GCM_256)
#define IEEE80211_CRYPTO_BITS \ #define IEEE80211_CRYPTO_BITS \
"\20\1WEP\2TKIP\3AES\4AES_CCM\5TKIPMIC\6CKIP" "\20\1WEP\2TKIP\3AES\4AES_CCM\5TKIPMIC\6CKIP\10AES_CCM_256" \
"\11BIP_CMAC_128\12BIP_CMAC_256\13BIP_GMAC_128\14BIP_CMAC_256" \
"\15AES_GCM_128\16AES_GCM_256"
bzUnsubmitted
Done
Inline Actions

In reality there is no "\7NONE" as we do not define the IEEE80211_CRYPTO_NONE? Where do we want to be consistent: in the defines or the printf?

bz: In reality there is no "\7NONE" as we do not define the IEEE80211_CRYPTO_NONE? Where do we…
ccUnsubmitted
Done
Inline Actions

In C printf, this is what I get. Also the output in Eclipse IDE console is a little different than in a Mac terminal. Is the result what we expected?

printf("%s\n", IEEE80211_CRYPTO_BITS);

quoted result:
WEPTKIPAESAES_CCMTKIPMICCKIPNONEAES_CCM_256 BIP_CMAC_128
BIP_CMAC_256 BIP_GMAC_128 BIP_CMAC_256
AES_GCM_128AES_GCM_256

cc: In C printf, this is what I get. Also the output in Eclipse IDE console is a little different…
emasteUnsubmitted
Done
Inline Actions

For use with printf(9)'s %b:

The %b identifier expects two arguments: an int and a char *.  These are
used as a register value and a print mask for decoding bitmasks.  The
print mask is made up of two parts: the base and the arguments.  The base
value is the output base (radix) expressed as an octal value; for
example, \10 gives octal and \20 gives hexadecimal.  The arguments are
made up of a sequence of bit identifiers.  Each bit identifier begins
with an octal value which is the number of the bit (starting from 1) this
identifier describes.  The rest of the identifier is a string of
characters containing the name of the bit.  The string is terminated by
either the bit number at the start of the next bit identifier or NUL for
the last bit identifier.
emaste: For use with printf(9)'s %b: ``` The %b identifier expects two arguments: an int and a…
ccUnsubmitted
Done
Inline Actions

For use with printf(9)'s %b:

The %b identifier expects two arguments: an int and a char *.  These are

Thanks for letting me know what I am missing. But after applying this patch in main, the ifconfig does not show the "cryptocaps". In verbose mode, it is empty. Any idea if there is a bug?

before the patch:
root@n2fbsd:/usr/src # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=b<WEP,TKIP,AES_CCM>

after the patch:
root@n2fbsd:~ # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
root@n2fbsd:~ #

root@n2fbsd:~ # ifconfig -v wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=0<>
htcaps=0<>
vhtcaps=0<>
...

cc: > For use with printf(9)'s %b: > ``` > The %b identifier expects two arguments: an int and…
ccUnsubmitted
Done
Inline Actions

Hold on a second. I think I forgot to compile the kernel with the HW CRYPTO defined. Let me test again.

cc: Hold on a second. I think I forgot to compile the kernel with the HW CRYPTO defined. Let me…
ccUnsubmitted
Done
Inline Actions

No surprise. To list it, HW CRYPTO must be defined for my driver.

After the patch:

root@n2fbsd:~ # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=b<WEP,TKIP,AES_CCM>
root@n2fbsd:~ #

cc: No surprise. To list it, HW CRYPTO must be defined for my driver. After the patch: >…
#if defined(__KERNEL__) || defined(_KERNEL) #if defined(__KERNEL__) || defined(_KERNEL)
struct ieee80211com; struct ieee80211com;
struct ieee80211vap; struct ieee80211vap;
struct ieee80211_node; struct ieee80211_node;
struct mbuf; struct mbuf;
MALLOC_DECLARE(M_80211_CRYPTO); MALLOC_DECLARE(M_80211_CRYPTO);
▲ Show 20 Lines • Show All 97 Lines • Show Last 20 Lines
View Options

sys/netinet/tcp_ecn.c

Show First 20 Lines • Show All 131 Lines • ▼ Show 20 Lines
*/ */
#define IEEE80211_CIPHER_WEP 0 #define IEEE80211_CIPHER_WEP 0
#define IEEE80211_CIPHER_TKIP 1 #define IEEE80211_CIPHER_TKIP 1
#define IEEE80211_CIPHER_AES_OCB 2 #define IEEE80211_CIPHER_AES_OCB 2
#define IEEE80211_CIPHER_AES_CCM 3 #define IEEE80211_CIPHER_AES_CCM 3
#define IEEE80211_CIPHER_TKIPMIC 4 /* TKIP MIC capability */ #define IEEE80211_CIPHER_TKIPMIC 4 /* TKIP MIC capability */
#define IEEE80211_CIPHER_CKIP 5 #define IEEE80211_CIPHER_CKIP 5
#define IEEE80211_CIPHER_NONE 6 /* pseudo value */ #define IEEE80211_CIPHER_NONE 6 /* pseudo value */
#define IEEE80211_CIPHER_AES_CCM_256 7
#define IEEE80211_CIPHER_BIP_CMAC_128 8
#define IEEE80211_CIPHER_BIP_CMAC_256 9
#define IEEE80211_CIPHER_BIP_GMAC_128 10
#define IEEE80211_CIPHER_BIP_GMAC_256 11
#define IEEE80211_CIPHER_AES_GCM_128 12
#define IEEE80211_CIPHER_AES_GCM_256 13
bzUnsubmitted
Not Done
Inline Actions

I love enums.

bz: I love enums.
ccUnsubmitted
Not Done
Inline Actions

I love enums.

+1

cc: > I love enums. +1
adrianAuthorUnsubmitted
Done
Inline Actions

I do too, but they're already defines. I'm happy to add it to a follow-up enum cleanup task?

We just need to be sure there's nothing in the codebase and wpa/hostapd driver_bsd.c code that's using these things as compile time flags?

adrian: I do too, but they're already defines. I'm happy to add it to a follow-up enum cleanup task?
#define IEEE80211_CIPHER_MAX (IEEE80211_CIPHER_NONE+1) #define IEEE80211_CIPHER_LAST 13
#define IEEE80211_CIPHER_MAX (IEEE80211_CIPHER_LAST+1)
/* capability bits in ic_cryptocaps/iv_cryptocaps */ /* capability bits in ic_cryptocaps/iv_cryptocaps */
#define IEEE80211_CRYPTO_WEP (1<<IEEE80211_CIPHER_WEP) #define IEEE80211_CRYPTO_WEP (1<<IEEE80211_CIPHER_WEP)
#define IEEE80211_CRYPTO_TKIP (1<<IEEE80211_CIPHER_TKIP) #define IEEE80211_CRYPTO_TKIP (1<<IEEE80211_CIPHER_TKIP)
#define IEEE80211_CRYPTO_AES_OCB (1<<IEEE80211_CIPHER_AES_OCB) #define IEEE80211_CRYPTO_AES_OCB (1<<IEEE80211_CIPHER_AES_OCB)
#define IEEE80211_CRYPTO_AES_CCM (1<<IEEE80211_CIPHER_AES_CCM) #define IEEE80211_CRYPTO_AES_CCM (1<<IEEE80211_CIPHER_AES_CCM)
#define IEEE80211_CRYPTO_TKIPMIC (1<<IEEE80211_CIPHER_TKIPMIC) #define IEEE80211_CRYPTO_TKIPMIC (1<<IEEE80211_CIPHER_TKIPMIC)
#define IEEE80211_CRYPTO_CKIP (1<<IEEE80211_CIPHER_CKIP) #define IEEE80211_CRYPTO_CKIP (1<<IEEE80211_CIPHER_CKIP)
#define IEEE80211_CRYPTO_AES_CCM_256 (1<<IEEE80211_CIPHER_AES_CCM_256)
#define IEEE80211_CRYPTO_BIP_CMAC_128 (1<<IEEE80211_CIPHER_BIP_CMAC_128)
#define IEEE80211_CRYPTO_BIP_CMAC_256 (1<<IEEE80211_CIPHER_BIP_CMAC_256)
#define IEEE80211_CRYPTO_BIP_GMAC_128 (1<<IEEE80211_CIPHER_BIP_GMAC_128)
#define IEEE80211_CRYPTO_BIP_GMAC_256 (1<<IEEE80211_CIPHER_BIP_GMAC_256)
#define IEEE80211_CRYPTO_AES_GCM_128 (1<<IEEE80211_CIPHER_AES_GCM_128)
#define IEEE80211_CRYPTO_AES_GCM_256 (1<<IEEE80211_CIPHER_AES_GCM_256)
#define IEEE80211_CRYPTO_BITS \ #define IEEE80211_CRYPTO_BITS \
"\20\1WEP\2TKIP\3AES\4AES_CCM\5TKIPMIC\6CKIP" "\20\1WEP\2TKIP\3AES\4AES_CCM\5TKIPMIC\6CKIP\10AES_CCM_256" \
"\11BIP_CMAC_128\12BIP_CMAC_256\13BIP_GMAC_128\14BIP_CMAC_256" \
"\15AES_GCM_128\16AES_GCM_256"
bzUnsubmitted
Done
Inline Actions

In reality there is no "\7NONE" as we do not define the IEEE80211_CRYPTO_NONE? Where do we want to be consistent: in the defines or the printf?

bz: In reality there is no "\7NONE" as we do not define the IEEE80211_CRYPTO_NONE? Where do we…
ccUnsubmitted
Done
Inline Actions

In C printf, this is what I get. Also the output in Eclipse IDE console is a little different than in a Mac terminal. Is the result what we expected?

printf("%s\n", IEEE80211_CRYPTO_BITS);

quoted result:
WEPTKIPAESAES_CCMTKIPMICCKIPNONEAES_CCM_256 BIP_CMAC_128
BIP_CMAC_256 BIP_GMAC_128 BIP_CMAC_256
AES_GCM_128AES_GCM_256

cc: In C printf, this is what I get. Also the output in Eclipse IDE console is a little different…
emasteUnsubmitted
Done
Inline Actions

For use with printf(9)'s %b:

The %b identifier expects two arguments: an int and a char *.  These are
used as a register value and a print mask for decoding bitmasks.  The
print mask is made up of two parts: the base and the arguments.  The base
value is the output base (radix) expressed as an octal value; for
example, \10 gives octal and \20 gives hexadecimal.  The arguments are
made up of a sequence of bit identifiers.  Each bit identifier begins
with an octal value which is the number of the bit (starting from 1) this
identifier describes.  The rest of the identifier is a string of
characters containing the name of the bit.  The string is terminated by
either the bit number at the start of the next bit identifier or NUL for
the last bit identifier.
emaste: For use with printf(9)'s %b: ``` The %b identifier expects two arguments: an int and a…
ccUnsubmitted
Done
Inline Actions

For use with printf(9)'s %b:

The %b identifier expects two arguments: an int and a char *.  These are

Thanks for letting me know what I am missing. But after applying this patch in main, the ifconfig does not show the "cryptocaps". In verbose mode, it is empty. Any idea if there is a bug?

before the patch:
root@n2fbsd:/usr/src # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=b<WEP,TKIP,AES_CCM>

after the patch:
root@n2fbsd:~ # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
root@n2fbsd:~ #

root@n2fbsd:~ # ifconfig -v wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=0<>
htcaps=0<>
vhtcaps=0<>
...

cc: > For use with printf(9)'s %b: > ``` > The %b identifier expects two arguments: an int and…
ccUnsubmitted
Done
Inline Actions

Hold on a second. I think I forgot to compile the kernel with the HW CRYPTO defined. Let me test again.

cc: Hold on a second. I think I forgot to compile the kernel with the HW CRYPTO defined. Let me…
ccUnsubmitted
Done
Inline Actions

No surprise. To list it, HW CRYPTO must be defined for my driver.

After the patch:

root@n2fbsd:~ # ifconfig wlan0 list caps
drivercaps=581c001<STA,SHSLOT,SHPREAMBLE,MONITOR,WPA1,WPA2,WME>
cryptocaps=b<WEP,TKIP,AES_CCM>
root@n2fbsd:~ #

cc: No surprise. To list it, HW CRYPTO must be defined for my driver. After the patch: >…
#if defined(__KERNEL__) || defined(_KERNEL) #if defined(__KERNEL__) || defined(_KERNEL)
struct ieee80211com; struct ieee80211com;
struct ieee80211vap; struct ieee80211vap;
struct ieee80211_node; struct ieee80211_node;
struct mbuf; struct mbuf;
MALLOC_DECLARE(M_80211_CRYPTO); MALLOC_DECLARE(M_80211_CRYPTO);
▲ Show 20 Lines • Show All 97 Lines • Show Last 20 Lines