select call in devd

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

select call in devd

Eitan Adler-4
Hi all,

select(2) is declared with restrict for the pointers for fd. Can y'all
confirm this is the correct fix?

It is only lightly tested (it seems to do its job on my machine)

Index: devd.cc
===================================================================
--- devd.cc (revision 329192)
+++ devd.cc (working copy)
@@ -1021,7 +1021,7 @@ event_loop(void)
  tv.tv_usec = 0;
  FD_ZERO(&fds);
  FD_SET(fd, &fds);
- rv = select(fd + 1, &fds, &fds, &fds, &tv);
+ rv = select(fd + 1, &fds, NULL, NULL, &tv);
  // No events -> we've processed all pending events
  if (rv == 0) {
  devdlog(LOG_DEBUG, "Calling daemon\n");


--
Eitan Adler
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Gleb Popov
On Wed, Feb 14, 2018 at 10:13 AM, Eitan Adler <[hidden email]> wrote:

> Hi all,
>
> select(2) is declared with restrict for the pointers for fd. Can y'all
> confirm this is the correct fix?
>
> It is only lightly tested (it seems to do its job on my machine)
>
> Index: devd.cc
> ===================================================================
> --- devd.cc (revision 329192)
> +++ devd.cc (working copy)
> @@ -1021,7 +1021,7 @@ event_loop(void)
>   tv.tv_usec = 0;
>   FD_ZERO(&fds);
>   FD_SET(fd, &fds);
> - rv = select(fd + 1, &fds, &fds, &fds, &tv);
> + rv = select(fd + 1, &fds, NULL, NULL, &tv);
>   // No events -> we've processed all pending events
>   if (rv == 0) {
>   devdlog(LOG_DEBUG, "Calling daemon\n");
>
>
> --
> Eitan Adler
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>

Haven't looked at code at all, but why is it using select and not
kqueue/kevent?
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Conrad Meyer-2
On Tue, Feb 13, 2018 at 11:56 PM, Gleb Popov <[hidden email]> wrote:
> Haven't looked at code at all, but why is it using select and not
> kqueue/kevent?

Because select/poll are a lot easier to use for one-off monitoring of
a single descriptor, and devd isn't trying to scale to 10k descriptors
here.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Eric van Gyzen-2
In reply to this post by Eitan Adler-4
On 02/14/2018 01:13, Eitan Adler wrote:

> Hi all,
>
> select(2) is declared with restrict for the pointers for fd. Can y'all
> confirm this is the correct fix?
>
> It is only lightly tested (it seems to do its job on my machine)
>
> Index: devd.cc
> ===================================================================
> --- devd.cc (revision 329192)
> +++ devd.cc (working copy)
> @@ -1021,7 +1021,7 @@ event_loop(void)
>   tv.tv_usec = 0;
>   FD_ZERO(&fds);
>   FD_SET(fd, &fds);
> - rv = select(fd + 1, &fds, &fds, &fds, &tv);
> + rv = select(fd + 1, &fds, NULL, NULL, &tv);
>   // No events -> we've processed all pending events
>   if (rv == 0) {
>   devdlog(LOG_DEBUG, "Calling daemon\n");

Looks good to me.

Eric
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Warner Losh
In reply to this post by Eitan Adler-4
On Wed, Feb 14, 2018 at 12:13 AM, Eitan Adler <[hidden email]> wrote:

> Hi all,
>
> select(2) is declared with restrict for the pointers for fd. Can y'all
> confirm this is the correct fix?
>

No. It's not. Select is not declared with restrict parameters. pselect is,
but select is not.

There's no real call to change it.

Warner


> It is only lightly tested (it seems to do its job on my machine)
>
> Index: devd.cc
> ===================================================================
> --- devd.cc (revision 329192)
> +++ devd.cc (working copy)
> @@ -1021,7 +1021,7 @@ event_loop(void)
>   tv.tv_usec = 0;
>   FD_ZERO(&fds);
>   FD_SET(fd, &fds);
> - rv = select(fd + 1, &fds, &fds, &fds, &tv);
> + rv = select(fd + 1, &fds, NULL, NULL, &tv);
>   // No events -> we've processed all pending events
>   if (rv == 0) {
>   devdlog(LOG_DEBUG, "Calling daemon\n");
>
>
> --
> Eitan Adler
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Eric van Gyzen-2
On 02/14/2018 09:13, Warner Losh wrote:

> On Wed, Feb 14, 2018 at 12:13 AM, Eitan Adler <[hidden email]> wrote:
>
>> Hi all,
>>
>> select(2) is declared with restrict for the pointers for fd. Can y'all
>> confirm this is the correct fix?
>>
>
> No. It's not. Select is not declared with restrict parameters. pselect is,
> but select is not.

You're right, it's not, but it /should/ be:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/select.html

        Issue 6
        The restrict keyword is added to the select() prototype for
        alignment with the ISO/IEC 9899:1999 standard.

> There's no real call to change it.

...unless he intends to mount a valiant effort to fix our declaration.

Eric
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Ian Lepore-3
On Wed, 2018-02-14 at 09:30 -0600, Eric van Gyzen wrote:

> On 02/14/2018 09:13, Warner Losh wrote:
> >
> > On Wed, Feb 14, 2018 at 12:13 AM, Eitan Adler <[hidden email]> wrote:
> >
> > >
> > > Hi all,
> > >
> > > select(2) is declared with restrict for the pointers for fd. Can y'all
> > > confirm this is the correct fix?
> > >
> > No. It's not. Select is not declared with restrict parameters. pselect is,
> > but select is not.
> You're right, it's not, but it /should/ be:
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/select.html
>
> Issue 6
> The restrict keyword is added to the select() prototype for
> alignment with the ISO/IEC 9899:1999 standard.
>
> >
> > There's no real call to change it.
> ...unless he intends to mount a valiant effort to fix our declaration.
>
> Eric

The fix isn't correct because of posix and/or a restrict keyword so
much as being the right thing to do because the only fd in the fdset is
open O_RDONLY, and it's not a socket that can provide OOB
notifications, so the fdset should be passed only as the readfds
argument because the other two types of events just can't happen.

-- Ian

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: select call in devd

Warner Losh
On Wed, Feb 14, 2018 at 9:35 AM, Ian Lepore <[hidden email]> wrote:

> On Wed, 2018-02-14 at 09:30 -0600, Eric van Gyzen wrote:
> > On 02/14/2018 09:13, Warner Losh wrote:
> > >
> > > On Wed, Feb 14, 2018 at 12:13 AM, Eitan Adler <[hidden email]>
> wrote:
> > >
> > > >
> > > > Hi all,
> > > >
> > > > select(2) is declared with restrict for the pointers for fd. Can
> y'all
> > > > confirm this is the correct fix?
> > > >
> > > No. It's not. Select is not declared with restrict parameters. pselect
> is,
> > > but select is not.
> > You're right, it's not, but it /should/ be:
> >
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/select.html
> >
> >       Issue 6
> >       The restrict keyword is added to the select() prototype for
> >       alignment with the ISO/IEC 9899:1999 standard.
> >
> > >
> > > There's no real call to change it.
> > ...unless he intends to mount a valiant effort to fix our declaration.
> >
> > Eric
>
> The fix isn't correct because of posix and/or a restrict keyword so
> much as being the right thing to do because the only fd in the fdset is
> open O_RDONLY, and it's not a socket that can provide OOB
> notifications, so the fdset should be passed only as the readfds
> argument because the other two types of events just can't happen.
>

Yea. Poll is likely a better interface.

Warner
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"