Date: Mon, 19 May 2014 23:02:54 +0400 From: Chagin Dmitry <dchagin@freebsd.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r266316 - in user/dchagin/lemul/sys: amd64/linux amd64/linux32 compat/linux i386/linux sys Message-ID: <20140519190254.GA90656@dchagin.static.corbina.net> In-Reply-To: <20140518124453.GA3555@stack.nl> References: <201405171439.s4HEdedl051853@svn.freebsd.org> <20140518124453.GA3555@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
--xHFwDpU9dbj6ez1V Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, May 18, 2014 at 02:44:53PM +0200, Jilles Tjoelker wrote: > On Sat, May 17, 2014 at 02:39:40PM +0000, Dmitry Chagin wrote: > > Author: dchagin > > Date: Sat May 17 14:39:40 2014 > > New Revision: 266316 > > URL: http://svnweb.freebsd.org/changeset/base/266316 >=20 > > Log: > > Implement eventfd system call. > > [snip] > > Modified: user/dchagin/lemul/sys/compat/linux/linux_event.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > --- user/dchagin/lemul/sys/compat/linux/linux_event.c Sat May 17 14:35:= 18 2014 (r266315) > > +++ user/dchagin/lemul/sys/compat/linux/linux_event.c Sat May 17 14:39:= 40 2014 (r266316) > > [snip] > > +static int > > +eventfd_read(struct file *fp, struct uio *uio, struct ucred *active_cr= ed, > > + int flags, struct thread *td) > > +{ > > + struct eventfd *efd; > > + eventfd_t count; > > + int error; > > + > > + efd =3D fp->f_data; > > + if (fp->f_type !=3D DTYPE_LINUXEFD || efd =3D=3D NULL) > > + return (EBADF); > > + > > + if (uio->uio_resid < sizeof(eventfd_t)) > > + return (EINVAL); > > + > > + error =3D 0; > > + mtx_lock(&efd->efd_lock); > > + if (efd->efd_count =3D=3D 0) { >=20 > This should be a while loop: a successful return of mtx_sleep() does not > guarantee anything about the predicate (for example if two threads are > trying to read). >=20 yes, agree. thanks > > + if ((efd->efd_flags & LINUX_O_NONBLOCK) !=3D 0) { >=20 > > + mtx_unlock(&efd->efd_lock); > > + return (EAGAIN); >=20 > These two lines could be: > error =3D EAGAIN; > break; >=20 > > + } > > + error =3D mtx_sleep(&efd->efd_count, &efd->efd_lock, PCATCH, "lefdrd= ", 0); > > + if (error =3D=3D ERESTART) > > + error =3D EINTR; >=20 > Why disable system call restart here? The Linux man page does not say > so, and it may cause subtle bugs in applications that do not expect an > [EINTR] failure. >=20 whoops, misreads linux code. thanks! > > + } > > + if (error =3D=3D 0) { > > + if ((efd->efd_flags & LINUX_EFD_SEMAPHORE) !=3D 0) { > > + count =3D 1; > > + --efd->efd_count; > > + } else { > > + count =3D efd->efd_count; > > + efd->efd_count =3D 0; > > + } > > + error =3D uiomove_nofault(&count, sizeof(eventfd_t), uio); >=20 > This uiomove_nofault() call will fail if the destination user buffer is > not physically. This may lead to almost irreproducible bugs. Instead, it > is better to unlock efd->efd_lock and then call uiomove(). >=20 > > + if (error =3D=3D 0) { > > + KNOTE_LOCKED(&efd->efd_sel.si_note, 0); > > + selwakeup(&efd->efd_sel); > > + wakeup(&efd->efd_count); > > + } >=20 > I don't think this block needs to be conditional on the success of > uiomove. The wakeups are because of the change to efd->efd_count which > happened anyway. Therefore, it can be moved to before uiomove. >=20 hm, looks like i overdid here )) as we are in reader context.. > > + } > > + mtx_unlock(&efd->efd_lock); > > + > > + return (error); > > +} > > + > > +static int > > +eventfd_write(struct file *fp, struct uio *uio, struct ucred *active_c= red, > > + int flags, struct thread *td) > > +{ > > + struct eventfd *efd; > > + eventfd_t count; > > + int error; > > + > > + efd =3D fp->f_data; > > + if (fp->f_type !=3D DTYPE_LINUXEFD || efd =3D=3D NULL) > > + return (EBADF); > > + > > + if (uio->uio_resid < sizeof(eventfd_t)) > > + return (EINVAL); > > + > > + error =3D uiomove(&count, sizeof(eventfd_t), uio); > > + if (error) > > + return (error); > > + if (count =3D=3D UINT64_MAX) > > + return (EINVAL); > > + > > + mtx_lock(&efd->efd_lock); > > +retry: > > + if (UINT64_MAX - efd->efd_count <=3D count) { > > + if ((efd->efd_flags & LINUX_O_NONBLOCK) !=3D 0) { > > + mtx_unlock(&efd->efd_lock); > > + return (EAGAIN); > > + } > > + error =3D mtx_sleep(&efd->efd_count, &efd->efd_lock, > > + PCATCH, "lefdwr", 0); >=20 > > + if (error =3D=3D ERESTART) > > + error =3D EINTR; >=20 > As in eventfd_read(), why disable system call restart here? >=20 > > + if (error =3D=3D 0) > > + goto retry; > > + } > > + if (error =3D=3D 0) { > > + efd->efd_count +=3D count; > > + KNOTE_LOCKED(&efd->efd_sel.si_note, 0); > > + selwakeup(&efd->efd_sel); > > + wakeup(&efd->efd_count); > > + } > > + mtx_unlock(&efd->efd_lock); > > + > > + return (error); > > +} > > + > > +static int > > +eventfd_poll(struct file *fp, int events, struct ucred *active_cred, > > + struct thread *td) > > +{ > > + struct eventfd *efd; > > + int revents =3D 0; > > + > > + efd =3D fp->f_data; > > + if (fp->f_type !=3D DTYPE_LINUXEFD || efd =3D=3D NULL) > > + return (POLLERR); > > + > > + mtx_lock(&efd->efd_lock); > > + if (efd->efd_count =3D=3D UINT64_MAX) > > + revents |=3D events & POLLERR; >=20 > POLLERR should be returned regardless of whether it is set in events. >=20 indeed. > > + if ((events & (POLLIN|POLLRDNORM)) && efd->efd_count > 0) > > + revents |=3D events & (POLLIN|POLLRDNORM); > > + if ((events & (POLLOUT|POLLWRNORM)) && UINT64_MAX - 1 > efd->efd_coun= t) > > + revents |=3D events & (POLLOUT|POLLWRNORM); > > + if (revents =3D=3D 0) > > + selrecord(td, &efd->efd_sel); > > + mtx_unlock(&efd->efd_lock); > > + > > + return (revents); > > +} > > [snip] > > +/*ARGSUSED*/ > > +static int > > +filt_eventfdread(struct knote *kn, long hint) > > +{ > > + struct eventfd *efd =3D kn->kn_hook; > > + int ret; > > + > > + mtx_assert(&efd->efd_lock, MA_OWNED); > > + ret =3D (efd->efd_count > 0); > > + > > + return (ret); > > +} > > + > > +/*ARGSUSED*/ > > +static int > > +filt_eventfdwrite(struct knote *kn, long hint) > > +{ > > + struct eventfd *efd =3D kn->kn_hook; > > + int ret; > > + > > + mtx_assert(&efd->efd_lock, MA_OWNED); > > + ret =3D (UINT64_MAX - 1 > efd->efd_count); >=20 > Perhaps set EV_EOF or EV_ERROR on overflow, so the epoll emulation can > reconstruct POLLERR. >=20 > Note, though, that sockets set EV_EOF when the corresponding direction > is closed, while POLLHUP and POLLERR should only be returned if both > directions are closed (otherwise, applications cannot properly finish > the other direction when one direction is closed). >=20 > > + > > + return (ret); > > +} > > [snip] > > +/*ARGSUSED*/ > > +static int > > +eventfd_stat(struct file *fp, struct stat *st, struct ucred *active_cr= ed, > > + struct thread *td) > > +{ > > + > > + return (ENXIO); > > +} >=20 > Linux appears to permit fstat(), though the returned data is rather > meaningless. >=20 no, did not find this in linux code. > > [snip] >=20 > --=20 > Jilles Tjoelker Thank you Jilles, glad to see that someone is interested. --=20 Have fun! chd --xHFwDpU9dbj6ez1V Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlN6VV4ACgkQ0t2Tb3OO/O3EFQCgoX+rt7Wbsp1VKdIbE+KxcgFN MW8Anj8fs2BgBqx7eC36oVxp5ZXTZVeP =gNCA -----END PGP SIGNATURE----- --xHFwDpU9dbj6ez1V--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140519190254.GA90656>