Date: Thu, 24 Aug 2023 16:31:28 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: af93fea71038 - main - timerfd: Move implementation from linux compat to sys/kern Message-ID: <CANCZdfo0_K6SedMfEAAkWFMFXMd146WyWKUJzzytGVfKA_8ZDg@mail.gmail.com> In-Reply-To: <ZOfXKlzSG0jYDU53@kib.kiev.ua> References: <202308242029.37OKTmVs091755@gitrepo.freebsd.org> <ZOfXKlzSG0jYDU53@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Thu, Aug 24, 2023 at 4:18 PM Konstantin Belousov <kostikbel@gmail.com> wrote: > On Thu, Aug 24, 2023 at 08:29:48PM +0000, Warner Losh wrote: > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=af93fea710385b2b11f0cabd377e7ed6f3d97c34 > > > > commit af93fea710385b2b11f0cabd377e7ed6f3d97c34 > > Author: Jake Freeland <jfree@freebsd.org> > > AuthorDate: 2023-08-24 04:39:54 +0000 > > Commit: Warner Losh <imp@FreeBSD.org> > > CommitDate: 2023-08-24 20:28:56 +0000 > > > > timerfd: Move implementation from linux compat to sys/kern > > > > Move the timerfd impelemntation from linux compat code to sys/kern. > Use > > it to implement the new system calls for timerfd. Add a hook to > kern_tc > > to allow timerfd to know when the system time has stepped. Add kqueue > > support to timerfd. Adjust a few names to be less Linux centric. > > > > RelNotes: YES > > Reviewed by: markj (on irc), imp, kib (with reservations), jhb > (slack) > > Differential Revision: https://reviews.freebsd.org/D38459 > >[snip] In my haste to get this in, I neglected to clear the commit message with Jake, and missed a few things that he refactored. He pointed out after I pushed the change: One minor complaint, though. Your commit message fails to recognize how much refactoring work I put into timerfd. I refactored a lot of important bits in the native version. The previous Linux-compat version was inaccurate and had functional inconsistencies with Linux's timerfd. Just to list them, I added: * Time jump handling through the TFD_TIMER_CANCEL_ON_SET flag. * Accuracy improvements (the Linux-compat implementation would deviate by seconds). * Guards for DoS attacks so timerfd could account for missed events. * Consistent error handling and expected return values that parody Linux. * Descriptor stat() support. for which I offer my profuse apologies for the oversight. > I did a very quick look over the added code. > > I do not see any protection for the timerfd_head list manipulation. > > It is not clear what is protected by tfd->tfd_lock: e.g. in timerfd_stat() > it covers reading of items, writing of which is not protected by the mtx, > everything except tfd_atim. > There is no annotations in the timer structure for the locking regime. > That's a good point. I'll work with Jake to add them. > stat st_ctim is always zero, this is somewhat strange. > I'm not sure what's going on there. > The > tfd = fp->f_data; > if (tfd == NULL || fp->f_type != DTYPE_TIMERFD) { > triggers UB when f_type is not DTYPE_TIMERFD. > Good catch. I didn't notice. > compat32 stuff was put into the sys/kern instead of sys/compat/freebsd32. > sys/timerfd.h pollutes userspace with sys/proc.h. > I don't think that was intentional. I didn't notice, but you are right on both of these. I'll work with Jake to fix. The second one is easier to fix. > The regenerated files were put in the same commit as (probably) human > written files, why? > That's my mistake. With brooks' new checks in Cirrus CI, I thought we'd transitioned to doing that in the same commit. I misunderstood. Warner [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 24, 2023 at 4:18 PM Konstantin Belousov <<a href="mailto:kostikbel@gmail.com">kostikbel@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Aug 24, 2023 at 08:29:48PM +0000, Warner Losh wrote:<br> > The branch main has been updated by imp:<br> > <br> > URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=af93fea710385b2b11f0cabd377e7ed6f3d97c34" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=af93fea710385b2b11f0cabd377e7ed6f3d97c34</a><br>; > <br> > commit af93fea710385b2b11f0cabd377e7ed6f3d97c34<br> > Author: Jake Freeland <<a href="mailto:jfree@freebsd.org" target="_blank">jfree@freebsd.org</a>><br> > AuthorDate: 2023-08-24 04:39:54 +0000<br> > Commit: Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2023-08-24 20:28:56 +0000<br> > <br> > timerfd: Move implementation from linux compat to sys/kern<br> > <br> > Move the timerfd impelemntation from linux compat code to sys/kern. Use<br> > it to implement the new system calls for timerfd. Add a hook to kern_tc<br> > to allow timerfd to know when the system time has stepped. Add kqueue<br> > support to timerfd. Adjust a few names to be less Linux centric.<br> > <br> > RelNotes: YES<br> > Reviewed by: markj (on irc), imp, kib (with reservations), jhb (slack)<br> > Differential Revision: <a href="https://reviews.freebsd.org/D38459" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D38459</a><br>; >[snip]</blockquote><div><br></div><div>In my haste to get this in, I neglected to clear the commit message with Jake, and missed a few things that</div><div>he refactored. He pointed out after I pushed the change:</div><div><div><br class="gmail-Apple-interchange-newline"></div></div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_quote"><div><div>One minor complaint, though. Your commit message fails to recognize</div></div></div><div class="gmail_quote"><div><div>how much refactoring work I put into timerfd. I refactored a lot of important</div></div></div><div class="gmail_quote"><div><div>bits in the native version. The previous Linux-compat version was inaccurate</div></div></div><div class="gmail_quote"><div><div>and had functional inconsistencies with Linux's timerfd.</div></div></div><div class="gmail_quote"><div><div><br></div></div></div><div class="gmail_quote"><div><div>Just to list them, I added:</div></div></div><div class="gmail_quote"><div><div>* Time jump handling through the TFD_TIMER_CANCEL_ON_SET flag.</div></div></div><div class="gmail_quote"><div><div>* Accuracy improvements (the Linux-compat implementation would deviate by seconds).</div></div></div><div class="gmail_quote"><div><div>* Guards for DoS attacks so timerfd could account for missed events.</div></div></div><div class="gmail_quote"><div><div>* Consistent error handling and expected return values that parody Linux.</div></div></div><div class="gmail_quote"><div><div>* Descriptor stat() support.</div></div></div></blockquote><div class="gmail_quote"><div><div><br></div><div>for which I offer my profuse apologies for the oversight.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I did a very quick look over the added code.<br> <br> I do not see any protection for the timerfd_head list manipulation.<br> <br> It is not clear what is protected by tfd->tfd_lock: e.g. in timerfd_stat()<br> it covers reading of items, writing of which is not protected by the mtx,<br> everything except tfd_atim.<br> There is no annotations in the timer structure for the locking regime.<br></blockquote><div><br></div><div>That's a good point. I'll work with Jake to add them.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> stat st_ctim is always zero, this is somewhat strange.<br></blockquote><div><br></div><div>I'm not sure what's going on there.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The<br> tfd = fp->f_data;<br> if (tfd == NULL || fp->f_type != DTYPE_TIMERFD) {<br> triggers UB when f_type is not DTYPE_TIMERFD.<br></blockquote><div><br></div><div>Good catch. I didn't notice.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> compat32 stuff was put into the sys/kern instead of sys/compat/freebsd32.<br> sys/timerfd.h pollutes userspace with sys/proc.h.<br></blockquote><div><br></div><div>I don't think that was intentional. I didn't notice, but you are right on both of these.</div><div>I'll work with Jake to fix. The second one is easier to fix.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The regenerated files were put in the same commit as (probably) human<br> written files, why?<br></blockquote><div><br></div><div>That's my mistake. With brooks' new checks in Cirrus CI, I thought we'd transitioned to</div><div>doing that in the same commit. I misunderstood.</div><div><br></div><div>Warner</div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfo0_K6SedMfEAAkWFMFXMd146WyWKUJzzytGVfKA_8ZDg>