[CFR] Kevent timer improvements

[Konstantin Belousov]

On Mon, Apr 14, 2014 at 08:19:37AM +0200, Baptiste Daroussin wrote:
> On Sun, Apr 13, 2014 at 11:20:38PM +0300, Konstantin Belousov wrote:
> > On Sun, Apr 13, 2014 at 09:19:28PM +0200, Baptiste Daroussin wrote:
> > > On Sun, Apr 13, 2014 at 05:20:28PM +0300, Konstantin Belousov wrote:
> > > > On Sun, Apr 13, 2014 at 03:15:51PM +0200, Baptiste Daroussin wrote:
> > > > > I have splitted my patch in multiple parts, let start with the first one:
> > > > > adding NOTE_NSECONDS, NOTE_USECONDS, NOTE_NSECONDS
> > > > > 
> > > > > http://people.freebsd.org/~bapt/kevent_timer.diff
> > > > 
> > > [...]
> > > > > +	else if (flags & NOTE_NSECONDS)
> > > > > +		modifier = SBT_1NS;
> > > > It is better to put the 'modifier = SBT_1MS;' statement as the else part.
> > > > 
> > > > That said, IMO it would be sometimes beneficial to have real flag to
> > > > specify milliseconds precision, in addition to milliseconds be the
> > > > default.
> > > 
> > > Done in the new patch
> > > > > +
> > > > > +		    timer2sbintime(kn->kn_sdata, kn->kn_sfflags), 0 /* 1ms? */,
> > > > There, at least the comment about precision should be updated.
> > > > But, it seems that for the seconds precision, it makes sense to
> > > > specify e.g. 1/2 sec as precision; or add an API flag to allow imprecise
> > > > callout scheduling.
> > > > 
> > > While I do agree this might be useful I do not have time to work on that right
> > > now so I just removed the comment which wasn't really accurate anyway.
> > > 
> > > Is that ok?
> > Leave XXX comment right near the zero argument, at least ?
> > 
> > > +	if (flags & NOTE_SECONDS)
> > > +		modifier = SBT_1S;
> > > +	else if (flags & NOTE_USECONDS)
> > > +		modifier = SBT_1US;
> > > +	else if (flags & NOTE_NSECONDS)
> > > +		modifier = SBT_1NS;
> > > +	else
> > > +		modifier = SBT_1MS;
> > There should be a case for NOTE_MSECONDS.
> > 
> ok I'll do
> > Hm, I think the checks should be made stronger. System should ensure
> > that only one flag is passed, and no invalid flags are supplied, right ?
> 
> I can check that only one flags is passed and no invalid flagrs are supplied but
> from what I see not a single part of kqueue(2) is checking that, so won't that
> be inconsistent with the rest of the API?
IMO we should move forward with the better practice.  I consider the
current lack of the checks as the deficiency, which hides the application
bugs.

See the RFTSIGNUM/RFTSIGFLAGS/RFTSIGMASK in unistd.h or
VM_ALLOC_COUNT/VM_ALLOC_COUNT_SHIFT for the idiomatic way to handle
similar needs.

> 
> I can probably check that only ona or 0 of the NOTE_*SECONDS is passed and
> return EINVAL in that case but no more.
> 
> (note that more NOTE_* are to be added later)
When added, the checks should be changed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140414/2fc9da8c/attachment.sig>