Date: Wed, 10 Apr 2024 13:44:59 +0100 From: Nuno Teixeira <eduardo@freebsd.org> To: tuexen@freebsd.org Cc: Drew Gallatin <gallatin@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, rrs <rrs@lakerest.net>, Mike Karels <mike@karels.net>, garyj@gmx.de, current@freebsd.org, net@freebsd.org, Randall Stewart <rrs@freebsd.org> Subject: Re: Request for Testing: TCP RACK Message-ID: <CAFDf7UK68POsmm%2BO74=6--cx1O-t3jsBrjgiaMV2r81otJq4gA@mail.gmail.com> In-Reply-To: <CAFDf7UK_6dZ1BRxqhLP0LmN%2BdBtX2=9OFRLZnc3Fv8b=nj-dGg@mail.gmail.com> References: <6e795e9c-8de4-4e02-9a96-8fabfaa4e66f@app.fastmail.com> <CAFDf7UKDWSnhm%2BTwP=ZZ9dkk0jmAgjGKPLpkX-CKuw3yH233gQ@mail.gmail.com> <CAFDf7UJq9SCnU-QYmS3t6EknP369w2LR0dNkQAc-NaRLvwVfoQ@mail.gmail.com> <A3F1FC0C-C199-4565-8E07-B233ED9E7B2E@freebsd.org> <6047C8EF-B1B0-4286-93FA-AA38F8A18656@karels.net> <ZfiI7GcbTwSG8kkO@kib.kiev.ua> <8031cd99-ded8-4b06-93b3-11cc729a8b2c@app.fastmail.com> <ZfiY-xUUM3wrBEz_@kib.kiev.ua> <38c54399-6c96-44d8-a3a2-3cc1bfbe50c2@app.fastmail.com> <27d8144f-0658-46f6-b8f3-35eb60061644@lakerest.net> <Zft8odA0s49eLhvk@kib.kiev.ua> <fc160c79-1884-4f68-8310-35e7ac0b9dd6@app.fastmail.com> <CAFDf7U%2Bsd0Hx6scDOvMuxOQaqVb8wi1ODrwT=uKDCqMn_81QEw@mail.gmail.com> <5C9863F7-0F1C-4D02-9F6D-9DDC5FBEB368@freebsd.org> <CAFDf7U%2BPtNhVsbR=pMhGVnxW5thWmzYBq9%2Bd4rSbZD0zRnJjkg@mail.gmail.com> <52479AA6-04F6-4D4A-ABE0-7142B47E28DF@freebsd.org> <CAFDf7UK_6dZ1BRxqhLP0LmN%2BdBtX2=9OFRLZnc3Fv8b=nj-dGg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] (...) Backup server is https://www.rsync.net/ (free 500GB for FreeBSD developers). Nuno Teixeira <eduardo@freebsd.org> escreveu (quarta, 10/04/2024 à(s) 13:39): > With base stack I can complete restic check successfully > downloading/reading/checking all files from a "big" remote compressed > backup. > Changing it to RACK stack, it fails. > > I run this command often because in the past, compression corruption > occured and this is the equivalent of restoring backup to check its > integrity. > > Maybe someone could do a restic test to check if this is reproducible. > > Thanks, > > > > <tuexen@freebsd.org> escreveu (quarta, 10/04/2024 à(s) 13:12): > >> >> >> > On 10. Apr 2024, at 13:40, Nuno Teixeira <eduardo@freebsd.org> wrote: >> > >> > Hello all, >> > >> > @ current 1500018 and fetching torrents with net-p2p/qbittorrent >> finished ~2GB download and connection UP until the end: >> > >> > --- >> > Apr 10 11:26:46 leg kernel: re0: watchdog timeout >> > Apr 10 11:26:46 leg kernel: re0: link state changed to DOWN >> > Apr 10 11:26:49 leg dhclient[58810]: New IP Address (re0): 192.168.1.67 >> > Apr 10 11:26:49 leg dhclient[58814]: New Subnet Mask (re0): >> 255.255.255.0 >> > Apr 10 11:26:49 leg dhclient[58818]: New Broadcast Address (re0): >> 192.168.1.255 >> > Apr 10 11:26:49 leg kernel: re0: link state changed to UP >> > Apr 10 11:26:49 leg dhclient[58822]: New Routers (re0): 192.168.1.1 >> > --- >> > >> > In the past tests, I've got more watchdog timeouts, connection goes >> down and a reboot needed to put it back (`service netif restart` didn't >> work). >> > >> > Other way to reproduce this is using sysutils/restic (backup program) >> to read/check all files from a remote server via sftp: >> > >> > `restic -r sftp:user@remote:restic-repo check --read-data` from a 60GB >> compressed backup. >> > >> > --- >> > watchdog timeout x3 as above >> > --- >> > >> > restic check fail log @ 15% progress: >> > --- >> > <snip> >> > Load(<data/52e2923dd6>, 17310001, 0) returned error, retrying after >> 1.7670599s: connection lost >> > Load(<data/d27a0abe0f>, 17456892, 0) returned error, retrying after >> 4.619104908s: connection lost >> > Load(<data/52e2923dd6>, 17310001, 0) returned error, retrying after >> 5.477648517s: connection lost >> > List(lock) returned error, retrying after 293.057766ms: connection lost >> > List(lock) returned error, retrying after 385.206693ms: connection lost >> > List(lock) returned error, retrying after 1.577594281s: connection lost >> > <snip> >> > >> > Connection continues UP. >> Hi, >> >> I'm not sure what the issue is you are reporting. Could you state >> what behavior you are experiencing with the base stack and with >> the RACK stack. In particular, what the difference is? >> >> Best regards >> Michael >> > >> > Cheers, >> > >> > <tuexen@freebsd.org> escreveu (quinta, 28/03/2024 à(s) 15:53): >> >> On 28. Mar 2024, at 15:00, Nuno Teixeira <eduardo@freebsd.org> wrote: >> >> >> >> Hello all! >> >> >> >> Running rack @b7b78c1c169 "Optimize HPTS..." very happy on my laptop >> (amd64)! >> >> >> >> Thanks all! >> > Thanks for the feedback! >> > >> > Best regards >> > Michael >> >> >> >> Drew Gallatin <gallatin@freebsd.org> escreveu (quinta, 21/03/2024 >> à(s) 12:58): >> >> The entire point is to *NOT* go through the overhead of scheduling >> something asynchronously, but to take advantage of the fact that a >> user/kernel transition is going to trash the cache anyway. >> >> >> >> In the common case of a system which has less than the threshold >> number of connections , we access the tcp_hpts_softclock function pointer, >> make one function call, and access hpts_that_need_softclock, and then >> return. So that's 2 variables and a function call. >> >> >> >> I think it would be preferable to avoid that call, and to move the >> declaration of tcp_hpts_softclock and hpts_that_need_softclock so that they >> are in the same cacheline. Then we'd be hitting just a single line in the >> common case. (I've made comments on the review to that effect). >> >> >> >> Also, I wonder if the threshold could get higher by default, so that >> hpts is never called in this context unless we're to the point where we're >> scheduling thousands of runs of the hpts thread (and taking all those clock >> interrupts). >> >> >> >> Drew >> >> >> >> On Wed, Mar 20, 2024, at 8:17 PM, Konstantin Belousov wrote: >> >>> On Tue, Mar 19, 2024 at 06:19:52AM -0400, rrs wrote: >> >>>> Ok I have created >> >>>> >> >>>> https://reviews.freebsd.org/D44420 >> >>>> >> >>>> >> >>>> To address the issue. I also attach a short version of the patch >> that Nuno >> >>>> can try and validate >> >>>> >> >>>> it works. Drew you may want to try this and validate the >> optimization does >> >>>> kick in since I can >> >>>> >> >>>> only now test that it does not on my local box :) >> >>> The patch still causes access to all cpu's cachelines on each userret. >> >>> It would be much better to inc/check the threshold and only schedule >> the >> >>> call when exceeded. Then the call can occur in some dedicated >> context, >> >>> like per-CPU thread, instead of userret. >> >>> >> >>>> >> >>>> >> >>>> R >> >>>> >> >>>> >> >>>> >> >>>> On 3/18/24 3:42 PM, Drew Gallatin wrote: >> >>>>> No. The goal is to run on every return to userspace for every >> thread. >> >>>>> >> >>>>> Drew >> >>>>> >> >>>>> On Mon, Mar 18, 2024, at 3:41 PM, Konstantin Belousov wrote: >> >>>>>> On Mon, Mar 18, 2024 at 03:13:11PM -0400, Drew Gallatin wrote: >> >>>>>>> I got the idea from >> >>>>>>> >> https://people.mpi-sws.org/~druschel/publications/soft-timers-tocs.pdf >> >>>>>>> The gist is that the TCP pacing stuff needs to run frequently, and >> >>>>>>> rather than run it out of a clock interrupt, its more efficient >> to run >> >>>>>>> it out of a system call context at just the point where we return >> to >> >>>>>>> userspace and the cache is trashed anyway. The current >> implementation >> >>>>>>> is fine for our workload, but probably not idea for a generic >> system. >> >>>>>>> Especially one where something is banging on system calls. >> >>>>>>> >> >>>>>>> Ast's could be the right tool for this, but I'm super unfamiliar >> with >> >>>>>>> them, and I can't find any docs on them. >> >>>>>>> >> >>>>>>> Would ast_register(0, ASTR_UNCOND, 0, func) be roughly equivalent >> to >> >>>>>>> what's happening here? >> >>>>>> This call would need some AST number added, and then it registers >> the >> >>>>>> ast to run on next return to userspace, for the current thread. >> >>>>>> >> >>>>>> Is it enough? >> >>>>>>> >> >>>>>>> Drew >> >>>>>> >> >>>>>>> >> >>>>>>> On Mon, Mar 18, 2024, at 2:33 PM, Konstantin Belousov wrote: >> >>>>>>>> On Mon, Mar 18, 2024 at 07:26:10AM -0500, Mike Karels wrote: >> >>>>>>>>> On 18 Mar 2024, at 7:04, tuexen@freebsd.org wrote: >> >>>>>>>>> >> >>>>>>>>>>> On 18. Mar 2024, at 12:42, Nuno Teixeira >> >>>>>> <eduardo@freebsd.org> wrote: >> >>>>>>>>>>> >> >>>>>>>>>>> Hello all! >> >>>>>>>>>>> >> >>>>>>>>>>> It works just fine! >> >>>>>>>>>>> System performance is OK. >> >>>>>>>>>>> Using patch on main-n268841-b0aaf8beb126(-dirty). >> >>>>>>>>>>> >> >>>>>>>>>>> --- >> >>>>>>>>>>> net.inet.tcp.functions_available: >> >>>>>>>>>>> Stack D >> >>>>>> Alias PCB count >> >>>>>>>>>>> freebsd freebsd 0 >> >>>>>>>>>>> rack * >> >>>>>> rack 38 >> >>>>>>>>>>> --- >> >>>>>>>>>>> >> >>>>>>>>>>> It would be so nice that we can have a sysctl tunnable for >> >>>>>> this patch >> >>>>>>>>>>> so we could do more tests without recompiling kernel. >> >>>>>>>>>> Thanks for testing! >> >>>>>>>>>> >> >>>>>>>>>> @gallatin: can you come up with a patch that is acceptable >> >>>>>> for Netflix >> >>>>>>>>>> and allows to mitigate the performance regression. >> >>>>>>>>> >> >>>>>>>>> Ideally, tcphpts could enable this automatically when it >> >>>>>> starts to be >> >>>>>>>>> used (enough?), but a sysctl could select auto/on/off. >> >>>>>>>> There is already a well-known mechanism to request execution of >> the >> >>>>>>>> specific function on return to userspace, namely AST. The >> difference >> >>>>>>>> with the current hack is that the execution is requested for one >> >>>>>> callback >> >>>>>>>> in the context of the specific thread. >> >>>>>>>> >> >>>>>>>> Still, it might be worth a try to use it; what is the reason to >> >>>>>> hit a thread >> >>>>>>>> that does not do networking, with TCP processing? >> >>>>>>>> >> >>>>>>>>> >> >>>>>>>>> Mike >> >>>>>>>>> >> >>>>>>>>>> Best regards >> >>>>>>>>>> Michael >> >>>>>>>>>>> >> >>>>>>>>>>> Thanks all! >> >>>>>>>>>>> Really happy here :) >> >>>>>>>>>>> >> >>>>>>>>>>> Cheers, >> >>>>>>>>>>> >> >>>>>>>>>>> Nuno Teixeira <eduardo@freebsd.org> escreveu (domingo, >> >>>>>> 17/03/2024 à(s) 20:26): >> >>>>>>>>>>>> >> >>>>>>>>>>>> Hello, >> >>>>>>>>>>>> >> >>>>>>>>>>>>> I don't have the full context, but it seems like the >> >>>>>> complaint is a performance regression in bonnie++ and perhaps other >> >>>>>> things when tcp_hpts is loaded, even when it is not used. Is that >> >>>>>> correct? >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> If so, I suspect its because we drive the >> >>>>>> tcp_hpts_softclock() routine from userret(), in order to avoid tons >> >>>>>> of timer interrupts and context switches. To test this theory, >> you >> >>>>>> could apply a patch like: >> >>>>>>>>>>>> >> >>>>>>>>>>>> It's affecting overall system performance, bonnie was just >> >>>>>> a way to >> >>>>>>>>>>>> get some numbers to compare. >> >>>>>>>>>>>> >> >>>>>>>>>>>> Tomorrow I will test patch. >> >>>>>>>>>>>> >> >>>>>>>>>>>> Thanks! >> >>>>>>>>>>>> >> >>>>>>>>>>>> -- >> >>>>>>>>>>>> Nuno Teixeira >> >>>>>>>>>>>> FreeBSD Committer (ports) >> >>>>>>>>>>> >> >>>>>>>>>>> >> >>>>>>>>>>> >> >>>>>>>>>>> -- >> >>>>>>>>>>> Nuno Teixeira >> >>>>>>>>>>> FreeBSD Committer (ports) >> >>>>>>>>> >> >>>>>>>> >> >>>>>> >> >>>>> >> >>> >> >>>> diff --git a/sys/netinet/tcp_hpts.c b/sys/netinet/tcp_hpts.c >> >>>> index 8c4d2d41a3eb..eadbee19f69c 100644 >> >>>> --- a/sys/netinet/tcp_hpts.c >> >>>> +++ b/sys/netinet/tcp_hpts.c >> >>>> @@ -216,6 +216,7 @@ struct tcp_hpts_entry { >> >>>> void *ie_cookie; >> >>>> uint16_t p_num; /* The hpts number one per cpu */ >> >>>> uint16_t p_cpu; /* The hpts CPU */ >> >>>> + uint8_t hit_callout_thresh; >> >>>> /* There is extra space in here */ >> >>>> /* Cache line 0x100 */ >> >>>> struct callout co __aligned(CACHE_LINE_SIZE); >> >>>> @@ -269,6 +270,11 @@ static struct hpts_domain_info { >> >>>> int cpu[MAXCPU]; >> >>>> } hpts_domains[MAXMEMDOM]; >> >>>> >> >>>> +counter_u64_t hpts_that_need_softclock; >> >>>> +SYSCTL_COUNTER_U64(_net_inet_tcp_hpts_stats, OID_AUTO, >> needsoftclock, CTLFLAG_RD, >> >>>> + &hpts_that_need_softclock, >> >>>> + "Number of hpts threads that need softclock"); >> >>>> + >> >>>> counter_u64_t hpts_hopelessly_behind; >> >>>> >> >>>> SYSCTL_COUNTER_U64(_net_inet_tcp_hpts_stats, OID_AUTO, hopeless, >> CTLFLAG_RD, >> >>>> @@ -334,7 +340,7 @@ SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, >> precision, CTLFLAG_RW, >> >>>> &tcp_hpts_precision, 120, >> >>>> "Value for PRE() precision of callout"); >> >>>> SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, cnt_thresh, CTLFLAG_RW, >> >>>> - &conn_cnt_thresh, 0, >> >>>> + &conn_cnt_thresh, DEFAULT_CONNECTION_THESHOLD, >> >>>> "How many connections (below) make us use the callout based >> mechanism"); >> >>>> SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, logging, CTLFLAG_RW, >> >>>> &hpts_does_tp_logging, 0, >> >>>> @@ -1548,6 +1554,9 @@ __tcp_run_hpts(void) >> >>>> struct tcp_hpts_entry *hpts; >> >>>> int ticks_ran; >> >>>> >> >>>> + if (counter_u64_fetch(hpts_that_need_softclock) == 0) >> >>>> + return; >> >>>> + >> >>>> hpts = tcp_choose_hpts_to_run(); >> >>>> >> >>>> if (hpts->p_hpts_active) { >> >>>> @@ -1683,6 +1692,13 @@ tcp_hpts_thread(void *ctx) >> >>>> ticks_ran = tcp_hptsi(hpts, 1); >> >>>> tv.tv_sec = 0; >> >>>> tv.tv_usec = hpts->p_hpts_sleep_time * HPTS_TICKS_PER_SLOT; >> >>>> + if ((hpts->p_on_queue_cnt > conn_cnt_thresh) && >> (hpts->hit_callout_thresh == 0)) { >> >>>> + hpts->hit_callout_thresh = 1; >> >>>> + counter_u64_add(hpts_that_need_softclock, 1); >> >>>> + } else if ((hpts->p_on_queue_cnt <= conn_cnt_thresh) && >> (hpts->hit_callout_thresh == 1)) { >> >>>> + hpts->hit_callout_thresh = 0; >> >>>> + counter_u64_add(hpts_that_need_softclock, -1); >> >>>> + } >> >>>> if (hpts->p_on_queue_cnt >= conn_cnt_thresh) { >> >>>> if(hpts->p_direct_wake == 0) { >> >>>> /* >> >>>> @@ -1818,6 +1834,7 @@ tcp_hpts_mod_load(void) >> >>>> cpu_top = NULL; >> >>>> #endif >> >>>> tcp_pace.rp_num_hptss = ncpus; >> >>>> + hpts_that_need_softclock = counter_u64_alloc(M_WAITOK); >> >>>> hpts_hopelessly_behind = counter_u64_alloc(M_WAITOK); >> >>>> hpts_loops = counter_u64_alloc(M_WAITOK); >> >>>> back_tosleep = counter_u64_alloc(M_WAITOK); >> >>>> @@ -2042,6 +2059,7 @@ tcp_hpts_mod_unload(void) >> >>>> free(tcp_pace.grps, M_TCPHPTS); >> >>>> #endif >> >>>> >> >>>> + counter_u64_free(hpts_that_need_softclock); >> >>>> counter_u64_free(hpts_hopelessly_behind); >> >>>> counter_u64_free(hpts_loops); >> >>>> counter_u64_free(back_tosleep); >> >>> >> >>> >> >> >> >> >> >> >> >> -- >> >> Nuno Teixeira >> >> FreeBSD Committer (ports) >> > >> > >> > >> > -- >> > Nuno Teixeira >> > FreeBSD Committer (ports) >> >> > > -- > Nuno Teixeira > FreeBSD Committer (ports) > -- Nuno Teixeira FreeBSD Committer (ports) [-- Attachment #2 --] <div dir="ltr"><div>(...)</div><div><br></div><div>Backup server is <a href="https://www.rsync.net/">https://www.rsync.net/</a>; (free 500GB for FreeBSD developers). <br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Nuno Teixeira <<a href="mailto:eduardo@freebsd.org">eduardo@freebsd.org</a>> escreveu (quarta, 10/04/2024 à(s) 13:39):<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>With base stack I can complete restic check successfully downloading/reading/checking all files from a "big" remote compressed backup.</div><div>Changing it to RACK stack, it fails.</div><div><br></div><div>I run this command often because in the past, compression corruption occured and this is the equivalent of restoring backup to check its integrity.</div><div><br></div><div>Maybe someone could do a restic test to check if this is reproducible.</div><div><br></div><div>Thanks,<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr"><<a href="mailto:tuexen@freebsd.org" target="_blank">tuexen@freebsd.org</a>> escreveu (quarta, 10/04/2024 à(s) 13:12):<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> <br> > On 10. Apr 2024, at 13:40, Nuno Teixeira <<a href="mailto:eduardo@freebsd.org" target="_blank">eduardo@freebsd.org</a>> wrote:<br> > <br> > Hello all,<br> > <br> > @ current 1500018 and fetching torrents with net-p2p/qbittorrent finished ~2GB download and connection UP until the end: <br> > <br> > ---<br> > Apr 10 11:26:46 leg kernel: re0: watchdog timeout<br> > Apr 10 11:26:46 leg kernel: re0: link state changed to DOWN<br> > Apr 10 11:26:49 leg dhclient[58810]: New IP Address (re0): 192.168.1.67<br> > Apr 10 11:26:49 leg dhclient[58814]: New Subnet Mask (re0): 255.255.255.0<br> > Apr 10 11:26:49 leg dhclient[58818]: New Broadcast Address (re0): 192.168.1.255<br> > Apr 10 11:26:49 leg kernel: re0: link state changed to UP<br> > Apr 10 11:26:49 leg dhclient[58822]: New Routers (re0): 192.168.1.1<br> > ---<br> > <br> > In the past tests, I've got more watchdog timeouts, connection goes down and a reboot needed to put it back (`service netif restart` didn't work).<br> > <br> > Other way to reproduce this is using sysutils/restic (backup program) to read/check all files from a remote server via sftp:<br>; > <br> > `restic -r sftp:user@remote:restic-repo check --read-data` from a 60GB compressed backup.<br> > <br> > ---<br> > watchdog timeout x3 as above<br> > ---<br> > <br> > restic check fail log @ 15% progress:<br> > ---<br> > <snip><br> > Load(<data/52e2923dd6>, 17310001, 0) returned error, retrying after 1.7670599s: connection lost<br> > Load(<data/d27a0abe0f>, 17456892, 0) returned error, retrying after 4.619104908s: connection lost<br> > Load(<data/52e2923dd6>, 17310001, 0) returned error, retrying after 5.477648517s: connection lost<br> > List(lock) returned error, retrying after 293.057766ms: connection lost<br> > List(lock) returned error, retrying after 385.206693ms: connection lost<br> > List(lock) returned error, retrying after 1.577594281s: connection lost<br> > <snip><br> > <br> > Connection continues UP.<br> Hi,<br> <br> I'm not sure what the issue is you are reporting. Could you state<br> what behavior you are experiencing with the base stack and with<br> the RACK stack. In particular, what the difference is?<br> <br> Best regards<br> Michael<br> > <br> > Cheers,<br> > <br> > <<a href="mailto:tuexen@freebsd.org" target="_blank">tuexen@freebsd.org</a>> escreveu (quinta, 28/03/2024 à(s) 15:53):<br> >> On 28. Mar 2024, at 15:00, Nuno Teixeira <<a href="mailto:eduardo@freebsd.org" target="_blank">eduardo@freebsd.org</a>> wrote:<br> >> <br> >> Hello all!<br> >> <br> >> Running rack @b7b78c1c169 "Optimize HPTS..." very happy on my laptop (amd64)!<br> >> <br> >> Thanks all!<br> > Thanks for the feedback!<br> > <br> > Best regards<br> > Michael<br> >> <br> >> Drew Gallatin <<a href="mailto:gallatin@freebsd.org" target="_blank">gallatin@freebsd.org</a>> escreveu (quinta, 21/03/2024 à(s) 12:58):<br> >> The entire point is to *NOT* go through the overhead of scheduling something asynchronously, but to take advantage of the fact that a user/kernel transition is going to trash the cache anyway.<br> >> <br> >> In the common case of a system which has less than the threshold number of connections , we access the tcp_hpts_softclock function pointer, make one function call, and access hpts_that_need_softclock, and then return. So that's 2 variables and a function call.<br> >> <br> >> I think it would be preferable to avoid that call, and to move the declaration of tcp_hpts_softclock and hpts_that_need_softclock so that they are in the same cacheline. Then we'd be hitting just a single line in the common case. (I've made comments on the review to that effect).<br> >> <br> >> Also, I wonder if the threshold could get higher by default, so that hpts is never called in this context unless we're to the point where we're scheduling thousands of runs of the hpts thread (and taking all those clock interrupts).<br> >> <br> >> Drew<br> >> <br> >> On Wed, Mar 20, 2024, at 8:17 PM, Konstantin Belousov wrote:<br> >>> On Tue, Mar 19, 2024 at 06:19:52AM -0400, rrs wrote:<br> >>>> Ok I have created<br> >>>> <br> >>>> <a href="https://reviews.freebsd.org/D44420" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D44420</a><br>; >>>> <br> >>>> <br> >>>> To address the issue. I also attach a short version of the patch that Nuno<br> >>>> can try and validate<br> >>>> <br> >>>> it works. Drew you may want to try this and validate the optimization does<br> >>>> kick in since I can<br> >>>> <br> >>>> only now test that it does not on my local box :)<br> >>> The patch still causes access to all cpu's cachelines on each userret.<br> >>> It would be much better to inc/check the threshold and only schedule the<br> >>> call when exceeded. Then the call can occur in some dedicated context,<br> >>> like per-CPU thread, instead of userret.<br> >>> <br> >>>> <br> >>>> <br> >>>> R<br> >>>> <br> >>>> <br> >>>> <br> >>>> On 3/18/24 3:42 PM, Drew Gallatin wrote:<br> >>>>> No. The goal is to run on every return to userspace for every thread.<br> >>>>> <br> >>>>> Drew<br> >>>>> <br> >>>>> On Mon, Mar 18, 2024, at 3:41 PM, Konstantin Belousov wrote:<br> >>>>>> On Mon, Mar 18, 2024 at 03:13:11PM -0400, Drew Gallatin wrote:<br> >>>>>>> I got the idea from<br> >>>>>>> <a href="https://people.mpi-sws.org/~druschel/publications/soft-timers-tocs.pdf" rel="noreferrer" target="_blank">https://people.mpi-sws.org/~druschel/publications/soft-timers-tocs.pdf</a><br>; >>>>>>> The gist is that the TCP pacing stuff needs to run frequently, and<br> >>>>>>> rather than run it out of a clock interrupt, its more efficient to run<br> >>>>>>> it out of a system call context at just the point where we return to<br> >>>>>>> userspace and the cache is trashed anyway. The current implementation<br> >>>>>>> is fine for our workload, but probably not idea for a generic system.<br> >>>>>>> Especially one where something is banging on system calls.<br> >>>>>>> <br> >>>>>>> Ast's could be the right tool for this, but I'm super unfamiliar with<br> >>>>>>> them, and I can't find any docs on them.<br> >>>>>>> <br> >>>>>>> Would ast_register(0, ASTR_UNCOND, 0, func) be roughly equivalent to<br> >>>>>>> what's happening here?<br> >>>>>> This call would need some AST number added, and then it registers the<br> >>>>>> ast to run on next return to userspace, for the current thread.<br> >>>>>> <br> >>>>>> Is it enough?<br> >>>>>>> <br> >>>>>>> Drew<br> >>>>>> <br> >>>>>>> <br> >>>>>>> On Mon, Mar 18, 2024, at 2:33 PM, Konstantin Belousov wrote:<br> >>>>>>>> On Mon, Mar 18, 2024 at 07:26:10AM -0500, Mike Karels wrote:<br> >>>>>>>>> On 18 Mar 2024, at 7:04, <a href="mailto:tuexen@freebsd.org" target="_blank">tuexen@freebsd.org</a> wrote:<br> >>>>>>>>> <br> >>>>>>>>>>> On 18. Mar 2024, at 12:42, Nuno Teixeira<br> >>>>>> <<a href="mailto:eduardo@freebsd.org" target="_blank">eduardo@freebsd.org</a>> wrote:<br> >>>>>>>>>>> <br> >>>>>>>>>>> Hello all!<br> >>>>>>>>>>> <br> >>>>>>>>>>> It works just fine!<br> >>>>>>>>>>> System performance is OK.<br> >>>>>>>>>>> Using patch on main-n268841-b0aaf8beb126(-dirty).<br> >>>>>>>>>>> <br> >>>>>>>>>>> ---<br> >>>>>>>>>>> net.inet.tcp.functions_available:<br> >>>>>>>>>>> Stack D<br> >>>>>> Alias PCB count<br> >>>>>>>>>>> freebsd freebsd 0<br> >>>>>>>>>>> rack *<br> >>>>>> rack 38<br> >>>>>>>>>>> ---<br> >>>>>>>>>>> <br> >>>>>>>>>>> It would be so nice that we can have a sysctl tunnable for<br> >>>>>> this patch<br> >>>>>>>>>>> so we could do more tests without recompiling kernel.<br> >>>>>>>>>> Thanks for testing!<br> >>>>>>>>>> <br> >>>>>>>>>> @gallatin: can you come up with a patch that is acceptable<br> >>>>>> for Netflix<br> >>>>>>>>>> and allows to mitigate the performance regression.<br> >>>>>>>>> <br> >>>>>>>>> Ideally, tcphpts could enable this automatically when it<br> >>>>>> starts to be<br> >>>>>>>>> used (enough?), but a sysctl could select auto/on/off.<br> >>>>>>>> There is already a well-known mechanism to request execution of the<br> >>>>>>>> specific function on return to userspace, namely AST. The difference<br> >>>>>>>> with the current hack is that the execution is requested for one<br> >>>>>> callback<br> >>>>>>>> in the context of the specific thread.<br> >>>>>>>> <br> >>>>>>>> Still, it might be worth a try to use it; what is the reason to<br> >>>>>> hit a thread<br> >>>>>>>> that does not do networking, with TCP processing?<br> >>>>>>>> <br> >>>>>>>>> <br> >>>>>>>>> Mike<br> >>>>>>>>> <br> >>>>>>>>>> Best regards<br> >>>>>>>>>> Michael<br> >>>>>>>>>>> <br> >>>>>>>>>>> Thanks all!<br> >>>>>>>>>>> Really happy here :)<br> >>>>>>>>>>> <br> >>>>>>>>>>> Cheers,<br> >>>>>>>>>>> <br> >>>>>>>>>>> Nuno Teixeira <<a href="mailto:eduardo@freebsd.org" target="_blank">eduardo@freebsd.org</a>> escreveu (domingo,<br> >>>>>> 17/03/2024 à(s) 20:26):<br> >>>>>>>>>>>> <br> >>>>>>>>>>>> Hello,<br> >>>>>>>>>>>> <br> >>>>>>>>>>>>> I don't have the full context, but it seems like the<br> >>>>>> complaint is a performance regression in bonnie++ and perhaps other<br> >>>>>> things when tcp_hpts is loaded, even when it is not used. Is that<br> >>>>>> correct?<br> >>>>>>>>>>>>> <br> >>>>>>>>>>>>> If so, I suspect its because we drive the<br> >>>>>> tcp_hpts_softclock() routine from userret(), in order to avoid tons<br> >>>>>> of timer interrupts and context switches. To test this theory, you<br> >>>>>> could apply a patch like:<br> >>>>>>>>>>>> <br> >>>>>>>>>>>> It's affecting overall system performance, bonnie was just<br> >>>>>> a way to<br> >>>>>>>>>>>> get some numbers to compare.<br> >>>>>>>>>>>> <br> >>>>>>>>>>>> Tomorrow I will test patch.<br> >>>>>>>>>>>> <br> >>>>>>>>>>>> Thanks!<br> >>>>>>>>>>>> <br> >>>>>>>>>>>> --<br> >>>>>>>>>>>> Nuno Teixeira<br> >>>>>>>>>>>> FreeBSD Committer (ports)<br> >>>>>>>>>>> <br> >>>>>>>>>>> <br> >>>>>>>>>>> <br> >>>>>>>>>>> --<br> >>>>>>>>>>> Nuno Teixeira<br> >>>>>>>>>>> FreeBSD Committer (ports)<br> >>>>>>>>> <br> >>>>>>>> <br> >>>>>> <br> >>>>> <br> >>> <br> >>>> diff --git a/sys/netinet/tcp_hpts.c b/sys/netinet/tcp_hpts.c<br> >>>> index 8c4d2d41a3eb..eadbee19f69c 100644<br> >>>> --- a/sys/netinet/tcp_hpts.c<br> >>>> +++ b/sys/netinet/tcp_hpts.c<br> >>>> @@ -216,6 +216,7 @@ struct tcp_hpts_entry {<br> >>>> void *ie_cookie;<br> >>>> uint16_t p_num; /* The hpts number one per cpu */<br> >>>> uint16_t p_cpu; /* The hpts CPU */<br> >>>> + uint8_t hit_callout_thresh;<br> >>>> /* There is extra space in here */<br> >>>> /* Cache line 0x100 */<br> >>>> struct callout co __aligned(CACHE_LINE_SIZE);<br> >>>> @@ -269,6 +270,11 @@ static struct hpts_domain_info {<br> >>>> int cpu[MAXCPU];<br> >>>> } hpts_domains[MAXMEMDOM];<br> >>>> <br> >>>> +counter_u64_t hpts_that_need_softclock;<br> >>>> +SYSCTL_COUNTER_U64(_net_inet_tcp_hpts_stats, OID_AUTO, needsoftclock, CTLFLAG_RD,<br> >>>> + &hpts_that_need_softclock,<br> >>>> + "Number of hpts threads that need softclock");<br> >>>> +<br> >>>> counter_u64_t hpts_hopelessly_behind;<br> >>>> <br> >>>> SYSCTL_COUNTER_U64(_net_inet_tcp_hpts_stats, OID_AUTO, hopeless, CTLFLAG_RD,<br> >>>> @@ -334,7 +340,7 @@ SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, precision, CTLFLAG_RW,<br> >>>> &tcp_hpts_precision, 120,<br> >>>> "Value for PRE() precision of callout");<br> >>>> SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, cnt_thresh, CTLFLAG_RW,<br> >>>> - &conn_cnt_thresh, 0,<br> >>>> + &conn_cnt_thresh, DEFAULT_CONNECTION_THESHOLD,<br> >>>> "How many connections (below) make us use the callout based mechanism");<br> >>>> SYSCTL_INT(_net_inet_tcp_hpts, OID_AUTO, logging, CTLFLAG_RW,<br> >>>> &hpts_does_tp_logging, 0,<br> >>>> @@ -1548,6 +1554,9 @@ __tcp_run_hpts(void)<br> >>>> struct tcp_hpts_entry *hpts;<br> >>>> int ticks_ran;<br> >>>> <br> >>>> + if (counter_u64_fetch(hpts_that_need_softclock) == 0)<br> >>>> + return;<br> >>>> +<br> >>>> hpts = tcp_choose_hpts_to_run();<br> >>>> <br> >>>> if (hpts->p_hpts_active) {<br> >>>> @@ -1683,6 +1692,13 @@ tcp_hpts_thread(void *ctx)<br> >>>> ticks_ran = tcp_hptsi(hpts, 1);<br> >>>> tv.tv_sec = 0;<br> >>>> tv.tv_usec = hpts->p_hpts_sleep_time * HPTS_TICKS_PER_SLOT;<br> >>>> + if ((hpts->p_on_queue_cnt > conn_cnt_thresh) && (hpts->hit_callout_thresh == 0)) {<br> >>>> + hpts->hit_callout_thresh = 1;<br> >>>> + counter_u64_add(hpts_that_need_softclock, 1);<br> >>>> + } else if ((hpts->p_on_queue_cnt <= conn_cnt_thresh) && (hpts->hit_callout_thresh == 1)) {<br> >>>> + hpts->hit_callout_thresh = 0;<br> >>>> + counter_u64_add(hpts_that_need_softclock, -1);<br> >>>> + }<br> >>>> if (hpts->p_on_queue_cnt >= conn_cnt_thresh) {<br> >>>> if(hpts->p_direct_wake == 0) {<br> >>>> /*<br> >>>> @@ -1818,6 +1834,7 @@ tcp_hpts_mod_load(void)<br> >>>> cpu_top = NULL;<br> >>>> #endif<br> >>>> tcp_pace.rp_num_hptss = ncpus;<br> >>>> + hpts_that_need_softclock = counter_u64_alloc(M_WAITOK);<br> >>>> hpts_hopelessly_behind = counter_u64_alloc(M_WAITOK);<br> >>>> hpts_loops = counter_u64_alloc(M_WAITOK);<br> >>>> back_tosleep = counter_u64_alloc(M_WAITOK);<br> >>>> @@ -2042,6 +2059,7 @@ tcp_hpts_mod_unload(void)<br> >>>> free(tcp_pace.grps, M_TCPHPTS);<br> >>>> #endif<br> >>>> <br> >>>> + counter_u64_free(hpts_that_need_softclock);<br> >>>> counter_u64_free(hpts_hopelessly_behind);<br> >>>> counter_u64_free(hpts_loops);<br> >>>> counter_u64_free(back_tosleep);<br> >>> <br> >>> <br> >> <br> >> <br> >> <br> >> -- <br> >> Nuno Teixeira<br> >> FreeBSD Committer (ports)<br> > <br> > <br> > <br> > -- <br> > Nuno Teixeira<br> > FreeBSD Committer (ports)<br> <br> </blockquote></div><br clear="all"><br><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><span style="color:rgb(102,102,102)">Nuno Teixeira<br>FreeBSD Committer (ports)</span></div></div> </blockquote></div><br clear="all"><br><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><span style="color:rgb(102,102,102)">Nuno Teixeira<br>FreeBSD Committer (ports)</span></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFDf7UK68POsmm%2BO74=6--cx1O-t3jsBrjgiaMV2r81otJq4gA>