net/tso: use the most restrictive segment size to perform the calculations
Needs RevisionPublic
Actions

Authored by royger on May 31 2016, 7:32 AM.

Details

Reviewers

gallatin
rrs
• hselasky

Group Reviewers

network
transport

Summary

The current TSO segment size accounting code was not taking into account
that the link segment size can be smaller than the maximum segment size
supported by the network card. Without this change, the if_hw_tsomaxsegcount
limit could be ignored, and the mbuf chain created by the TSO code could
indeed have more segments than supported.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4043
Build 4086: arc lint + arc unit

Event Timeline

royger updated this revision to Diff 17139.May 31 2016, 7:32 AM

royger retitled this revision from to net/tso: use the most restrictive segment size to perform the calculations.

royger updated this object.

royger edited the test plan for this revision. (Show Details)

Herald added a reviewer: transport. · View Herald TranscriptMay 31 2016, 7:32 AM

Herald added a subscriber: imp. · View Herald Transcript

royger added a reviewer: network.May 31 2016, 7:33 AM

royger mentioned this in D6612: net: allow fast-forwarding TSO packets.Jun 6 2016, 2:53 PM

rgrimes added a subscriber: rgrimes.Aug 6 2020, 5:21 PM

Herald added a subscriber: melifaro. · View Herald TranscriptAug 6 2020, 5:21 PM

rgrimes added a reviewer: gallatin.Aug 13 2020, 2:29 PM

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

rgrimes edited the test plan for this revision. (Show Details)Aug 13 2020, 11:06 PM

gallatin mentioned this in D6611: net: translate inbound checksum offloading flags to outbound when forwarding.Aug 15 2020, 7:46 PM

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

Ugh, I'm wrong. I was confusing t_tsomax with t_tsomaxsegsize. I forgot that some NICs are apparently so broken that they limit TSO seg sizes.

In D6656#578863, @gallatin wrote:

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

Ugh, I'm wrong. I was confusing t_tsomax with t_tsomaxsegsize. I forgot that some NICs are apparently so broken that they limit TSO seg sizes.

So this is correct? Or am I missing something else?

In D6656#579330, @royger wrote:

In D6656#578863, @gallatin wrote:

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

Ugh, I'm wrong. I was confusing t_tsomax with t_tsomaxsegsize. I forgot that some NICs are apparently so broken that they limit TSO seg sizes.

So this is correct? Or am I missing something else?

I think it is correct, but I would prefer somebody from the transport team to review it.

gallatin added reviewers: rrs, • hselasky.Aug 18 2020, 7:36 PM

Hi Drew,

t_tsomaxsegsize usually only affects the number of DMA segments and the mbuf chain. At least the Mellanox NICS do it this way.

How does t_maxseg affect the generated DMA chain?

--HPS

I mean DMA -> mbuf chain.

In D6656#579632, @hselasky wrote:

I mean DMA -> mbuf chain.

if_hw_tsomaxsegsize is used to calculate the size of the packets the NIC is going to emit, and tcp_m_copym uses it to solve other constraints (eg, if_hw_tsomaxsegcount)

I do think that this change is probably correct. If so, it needs to also be applied to rack and bbr.

(/rant) I honestly hate this weird impedance matching attempt between TSO and DMA. We should just do what linux does, and punt to software TSO for broken hardware that can't send something. (/end rant)

This revision now requires changes to proceed.Aug 25 2020, 10:55 PM

Revision Contents
Changeset List

Path

Size

sys/

netinet/

tcp_output.c

3 lines

Diff 17139

View Options

sys/netinet/tcp_output.c

Show First 20 Lines • Show All 146 Lines • ▼ Show 20 Lines	static const struct Algorithm_t Algorithm[] = {
{ "skein512", "Skein512", &SKEIN512_TestOutput,		{ "skein512", "Skein512", &SKEIN512_TestOutput,
(DIGEST_Init)&SKEIN512_Init, (DIGEST_Update)&SKEIN512_Update,		(DIGEST_Init)&SKEIN512_Init, (DIGEST_Update)&SKEIN512_Update,
(DIGEST_End*)&SKEIN512_End, &SKEIN512_Data, &SKEIN512_Fd },		(DIGEST_End*)&SKEIN512_End, &SKEIN512_Data, &SKEIN512_Fd },
{ "skein1024", "Skein1024", &SKEIN1024_TestOutput,		{ "skein1024", "Skein1024", &SKEIN1024_TestOutput,
(DIGEST_Init)&SKEIN1024_Init, (DIGEST_Update)&SKEIN1024_Update,		(DIGEST_Init)&SKEIN1024_Init, (DIGEST_Update)&SKEIN1024_Update,
(DIGEST_End*)&SKEIN1024_End, &SKEIN1024_Data, &SKEIN1024_Fd }		(DIGEST_End*)&SKEIN1024_End, &SKEIN1024_Data, &SKEIN1024_Fd }
};		};

		static unsigned digest;
		static unsigned malformed;
		static bool gnu_emu = false;

static void		static void
MD5_Update(MD5_CTX c, const unsigned char data, size_t len)		MD5_Update(MD5_CTX c, const unsigned char data, size_t len)
{		{
MD5Update(c, data, len);		MD5Update(c, data, len);
}		}

		struct chksumrec {
		char *filename;
		char *chksum;
		struct chksumrec *next;
		};

		static struct chksumrec *head = NULL;
		static struct chksumrec **next = &head;
		impUnsubmitted Done Inline Actions Can you replace this with some #define, or at the very least do the math for why 256 is the right length for 1024 bit hash? imp: Can you replace this with some #define, or at the very least do the math for why 256 is the…
		seAuthorUnsubmitted Done Inline Actions Can you replace this with some #define, or at the very least do the math for why 256 is the right length for 1024 bit hash? There already was HEX_DIGIT_LENGTH and I do use it now. se: > Can you replace this with some #define, or at the very least do the math for why 256 is the…

		impUnsubmitted Done Inline Actions USE MAXPATHLEN + 1 then. imp: USE MAXPATHLEN + 1 then.
		seAuthorUnsubmitted Done Inline Actions Code changed but does not use MAXPATHLEN in the definition of the longest expected input line. se: Code changed but does not use MAXPATHLEN in the definition of the longest expected input line.
		#define PADDING 7 /* extra padding for "SHA512t256 (...) = ...\n" style */
		#define CHKFILELINELEN (HEX_DIGEST_LENGTH + MAXPATHLEN + PADDING)

		static int gnu_check(const char *checksumsfile)
		{
		FILE *inp;
		char linebuf[CHKFILELINELEN];
		int linelen;
		int lineno;
		impUnsubmitted Not Done Inline Actions Does this work with filenames / paths with spaces in them? imp: Does this work with filenames / paths with spaces in them?
		char *filename;
		char *hashstr;
		struct chksumrec *rec;
		const char *digestname;
		int digestnamelen;
		int hashstrlen;

		if ((inp = fopen(checksumsfile, "r")) == NULL)
		err(1, "%s", checksumsfile);
		digestname = Algorithm[digest].name;
		digestnamelen = strlen(digestname);
		hashstrlen = strlen(*(Algorithm[digest].TestOutput[0]));
		lineno = 1;
		while (fgets(linebuf, sizeof(linebuf), inp) != NULL) {
		linelen = strlen(linebuf) - 1;
		if (linelen <= 0)
		break;
		if (linebuf[linelen] != '\n')
		errx(1, "malformed input line %d (len=%d)", lineno, linelen);
		linebuf[linelen] = '\0';
		filename = linebuf + digestnamelen + 2;
		hashstr = linebuf + linelen - hashstrlen;
		/*
		* supported formats:
		* BSD: <DigestName> (<Filename>): <Digest>
		* GNU: <Digest> [ *]<Filename>
		*/
		if (linelen >= digestnamelen + hashstrlen + 6 &&
		strncmp(linebuf, digestname, digestnamelen) == 0 &&
		strncmp(filename - 2, " (", 2) == 0 &&
		strncmp(hashstr - 4, ") = ", 4) == 0) {
		*(hashstr - 4) = '\0';
		} else if (linelen >= hashstrlen + 3 &&
		linebuf[hashstrlen] == ' ' &&
		(linebuf[hashstrlen] == ' ' \|\|
		linebuf[hashstrlen] == '*')) {
		linebuf[hashstrlen] = '\0';
		hashstr = linebuf;
		filename = linebuf + hashstrlen + 2;
		} else {
		malformed++;
		continue;
		}
		rec = malloc(sizeof (*rec));
		if (rec == NULL)
		errx(1, "malloc failed");
		rec->chksum = strdup(hashstr);
		rec->filename = strdup(filename);
		if (rec->chksum == NULL \|\| rec->filename == NULL)
		errx(1, "malloc failed");
		rec->next = NULL;
		*next = rec;
		next = &rec->next;
		lineno++;
		}
		fclose(inp);
		return (lineno - 1);
		}

/* Main driver.		/* Main driver.

Arguments (may be any combination):		Arguments (may be any combination):
-sstring - digests string		-sstring - digests string
-t - runs time trial		-t - runs time trial
-x - runs test script		-x - runs test script
filename - digests file		filename - digests file
(none) - digests standard input		(none) - digests standard input
*/		*/
int		int
main(int argc, char *argv[])		main(int argc, char *argv[])
{		{
#ifdef HAVE_CAPSICUM		#ifdef HAVE_CAPSICUM
cap_rights_t rights;		cap_rights_t rights;
#endif		#endif
int ch, fd;		int ch, fd;
char p, string;		char p, string;
char buf[HEX_DIGEST_LENGTH];		char buf[HEX_DIGEST_LENGTH];
size_t len;		size_t len;
unsigned digest;
char *progname;		char *progname;
bool gnu_emu = false;		struct chksumrec *rec;
		int numrecs;

if ((progname = strrchr(argv[0], '/')) == NULL)		if ((progname = strrchr(argv[0], '/')) == NULL)
progname = argv[0];		progname = argv[0];
else		else
progname++;		progname++;

/*		/*
* GNU coreutils has a number of programs named *sum. These produce		* GNU coreutils has a number of programs named *sum. These produce
* similar results to the BSD version, but in a different format,		* similar results to the BSD version, but in a different format,
* similar to BSD's -r flag. We install links to this program with		* similar to BSD's -r flag. We install links to this program with
* ending 'sum' to provide this compatibility. Check here to see if the		* ending 'sum' to provide this compatibility. Check here to see if the
* name of the program ends in 'sum', set the flag and drop the 'sum' so		* name of the program ends in 'sum', set the flag and drop the 'sum' so
* the digest lookup works. Also, make -t a nop when running in this mode		* the digest lookup works. Also, make -t a nop when running in this mode
* since that means 'text file' there (though it's a nop in coreutils		* since that means 'text file' there (though it's a nop in coreutils
* on unix-like systems). The -c flag conflicts, so it's just disabled		* on unix-like systems). The -c flag conflicts, so it's just disabled
* in this mode (though in the future it might be implemented).		* in this mode (though in the future it might be implemented).
*/		*/
len = strlen(progname);		len = strlen(progname);
if (len > 3 && strcmp(progname + len - 3, "sum") == 0) {		if (len > 3 && strcmp(progname + len - 3, "sum") == 0) {
progname[len - 3] = '\0';		len -= 3;
rflag = 1;		rflag = 1;
gnu_emu = true;		gnu_emu = true;
}		}

for (digest = 0; digest < sizeof(Algorithm)/sizeof(*Algorithm); digest++)		for (digest = 0; digest < sizeof(Algorithm)/sizeof(*Algorithm); digest++)
if (strcasecmp(Algorithm[digest].progname, progname) == 0)		if (strncasecmp(Algorithm[digest].progname, progname, len) == 0)
break;		break;

if (digest == sizeof(Algorithm)/sizeof(*Algorithm))		if (digest == sizeof(Algorithm)/sizeof(*Algorithm))
digest = 0;		digest = 0;

failed = 0;		failed = 0;
checkAgainst = NULL;		checkAgainst = NULL;
checksFailed = 0;		checksFailed = 0;
skip = 0;		skip = 0;
while ((ch = getopt(argc, argv, "bc:pqrs:tx")) != -1)		while ((ch = getopt(argc, argv, "bc:pqrs:tx")) != -1)
switch (ch) {		switch (ch) {
case 'b':		case 'b':
break;		break;
case 'c':		case 'c':
if (gnu_emu)		if (gnu_emu)
errx(1, "-c check option not supported");		numrecs = gnu_check(optarg);
		else
checkAgainst = optarg;		checkAgainst = optarg;
break;		break;
case 'p':		case 'p':
pflag = 1;		pflag = 1;
break;		break;
case 'q':		case 'q':
qflag = 1;		qflag = 1;
break;		break;
case 'r':		case 'r':
Show All 19 Lines	#endif
argc -= optind;		argc -= optind;
argv += optind;		argv += optind;

#ifdef HAVE_CAPSICUM		#ifdef HAVE_CAPSICUM
if (caph_limit_stdout() < 0 \|\| caph_limit_stderr() < 0)		if (caph_limit_stdout() < 0 \|\| caph_limit_stderr() < 0)
err(1, "unable to limit rights for stdio");		err(1, "unable to limit rights for stdio");
#endif		#endif

		if (gnu_emu) {
		argc = 0;
		argv = (char*)calloc(sizeof(char ), numrecs + 1);
		for (rec = head; rec != NULL; rec = rec->next) {
		argv[argc] = rec->filename;
		argc++;
		}
		rec = head;
		}

if (*argv) {		if (*argv) {
do {		do {
if ((fd = open(*argv, O_RDONLY)) < 0) {		if ((fd = open(*argv, O_RDONLY)) < 0) {
warn("%s", *argv);		warn("%s", *argv);
failed++;		failed++;
continue;		continue;
}		}
/*		/*
* XXX Enter capability mode on the last argv file.		* XXX Enter capability mode on the last argv file.
* When a casper file service or other approach is		* When a casper file service or other approach is
* available, switch to that and enter capability mode		* available, switch to that and enter capability mode
* earlier.		* earlier.
*/		*/
if (*(argv + 1) == NULL) {		if (*(argv + 1) == NULL) {
#ifdef HAVE_CAPSICUM		#ifdef HAVE_CAPSICUM
cap_rights_init(&rights, CAP_READ);		cap_rights_init(&rights, CAP_READ);
if (caph_rights_limit(fd, &rights) < 0 \|\|		if (caph_rights_limit(fd, &rights) < 0 \|\|
caph_enter() < 0)		caph_enter() < 0)
err(1, "capsicum");		err(1, "capsicum");
#endif		#endif
}		}
		if (gnu_emu) {
		checkAgainst = rec->chksum;
		rec = rec->next;
		}
p = Algorithm[digest].Fd(fd, buf);		p = Algorithm[digest].Fd(fd, buf);
(void)close(fd);		(void)close(fd);
MDOutput(&Algorithm[digest], p, argv);		MDOutput(&Algorithm[digest], p, argv);
} while (*++argv);		} while (*++argv);
} else if (!sflag && !skip) {		} else if (!sflag && !skip) {
#ifdef HAVE_CAPSICUM		#ifdef HAVE_CAPSICUM
if (caph_limit_stdin() < 0 \|\| caph_enter() < 0)		if (caph_limit_stdin() < 0 \|\| caph_enter() < 0)
err(1, "capsicum");		err(1, "capsicum");
#endif		#endif
p = MDFilter(&Algorithm[digest], (char *)&buf, pflag);		p = MDFilter(&Algorithm[digest], (char *)&buf, pflag);
MDOutput(&Algorithm[digest], p, NULL);		MDOutput(&Algorithm[digest], p, NULL);
} else if (sflag) {		} else if (sflag) {
len = strlen(string);		len = strlen(string);
p = Algorithm[digest].Data(string, len, buf);		p = Algorithm[digest].Data(string, len, buf);
MDOutput(&Algorithm[digest], p, &string);		MDOutput(&Algorithm[digest], p, &string);
}		}
		if (gnu_emu) {
		if (malformed > 0)
		warnx("WARNING: %d lines are improperly formatted", malformed);
		if (checksFailed > 0)
		warnx("WARNING: %d computed checksum did NOT match", checksFailed);
		}
if (failed != 0)		if (failed != 0)
return (1);		return (1);
if (checksFailed != 0)		if (checksFailed != 0)
return (2);		return (2);

return (0);		return (0);
}		}

/*		/*
* Common output handling		* Common output handling
*/		*/
static void		static void
MDOutput(const Algorithm_t alg, char p, char *argv[])		MDOutput(const Algorithm_t alg, char p, char *argv[])
{		{
		bool checkfailed = false;

if (p == NULL) {		if (p == NULL) {
warn("%s", *argv);		warn("%s", *argv);
failed++;		failed++;
} else {		} else {
/*		/*
* If argv is NULL we are reading from stdin, where the output		* If argv is NULL we are reading from stdin, where the output
* format has always been just the hash.		* format has always been just the hash.
*/		*/
if (qflag \|\| argv == NULL)		if (qflag \|\| argv == NULL) {
printf("%s", p);		printf("%s", p);
else if (rflag)		} else if (gnu_emu) {
		checkfailed = strcasecmp(checkAgainst, p) != 0;
		printf("%s: %s", *argv, checkfailed ? "FAILED" : "OK");
		} else {
		if (rflag)
printf("%s %s", p, *argv);		printf("%s %s", p, *argv);
else		else
printf("%s (%s) = %s",		printf("%s (%s) = %s", alg->name, *argv, p);
alg->name, *argv, p);		if (checkAgainst) {
if (checkAgainst && strcasecmp(checkAgainst, p) != 0)		checkfailed = strcasecmp(checkAgainst, p) != 0;
{		if (!qflag && checkfailed)
checksFailed++;
if (!qflag)
printf(" [ Failed ]");		printf(" [ Failed ]");
}		}
		}
printf("\n");		printf("\n");
}		}
		if (checkfailed)
		checksFailed++;
}		}

/*		/*
* Measures the time to digest TEST_BLOCK_COUNT TEST_BLOCK_LEN-byte blocks.		* Measures the time to digest TEST_BLOCK_COUNT TEST_BLOCK_LEN-byte blocks.
*/		*/
static void		static void
MDTimeTrial(const Algorithm_t *alg)		MDTimeTrial(const Algorithm_t *alg)
{		{
▲ Show 20 Lines • Show All 210 Lines • ▼ Show 20 Lines	MDFilter(const Algorithm_t alg, char buf, int tee)

return (p);		return (p);
}		}

static void		static void
usage(const Algorithm_t *alg)		usage(const Algorithm_t *alg)
{		{

		if (gnu_emu)
		fprintf(stderr, "usage: %ssum [-pqrtx] [-c file] [-s string] [files ...]\n", alg->progname);
		else
fprintf(stderr, "usage: %s [-pqrtx] [-c string] [-s string] [files ...]\n", alg->progname);		fprintf(stderr, "usage: %s [-pqrtx] [-c string] [-s string] [files ...]\n", alg->progname);
exit(1);		exit(1);
}		}

net/tso: use the most restrictive segment size to perform the calculationsNeeds RevisionPublicActions