[CFR] Kevent timer improvements

[Baptiste Daroussin]

On Mon, Apr 14, 2014 at 08:18:34PM +0300, Konstantin Belousov wrote:
> 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'll do thanks for tips!
> 
> > 
> > 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.

ok

I'll come back with a better patch.

regards,
Bapt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140414/75c78914/attachment.sig>