From owner-svn-src-user@FreeBSD.ORG Mon May 19 19:03:00 2014 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7A873D3D; Mon, 19 May 2014 19:03:00 +0000 (UTC) Received: from mail-lb0-x233.google.com (mail-lb0-x233.google.com [IPv6:2a00:1450:4010:c04::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id B9CD22B86; Mon, 19 May 2014 19:02:59 +0000 (UTC) Received: by mail-lb0-f179.google.com with SMTP id c11so4369974lbj.38 for ; Mon, 19 May 2014 12:02:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=7w9c2yZrJ2MZ9yKNTr5q7hLhIWVUZ8fenz1ar4wrhuQ=; b=l1rZoiuPK0sYTY1gxO9cKwoIZ5jYGXht0VjYasQAKsaKOI7yR5Q81Op7TwEhFgGL0N V4eN+g099rzunTAXEpbSCmPJcblj0nFPH4g5TAttxXbCHv1tSN6GQsB7FLt37NJTHs8k pxhBk975dJGx4JKcCTgiR6HsFUnuTdqTIFa8/zX8sxHk1GInProVjV2qC0ciwOVwut/t 5aJMS48JAfl6PmgaTsBXyBUGETtfj3sF3BX+cT0vEoGvQi+hL9s3YUwJisXknzU5tkWe fIm239if95+aS9Ju4XJOJxErTE73yac1vf7FdkawAcgQjUowlQ5iYkq72heFtEtZKH4l GrgA== X-Received: by 10.112.201.106 with SMTP id jz10mr3395603lbc.62.1400526177607; Mon, 19 May 2014 12:02:57 -0700 (PDT) Received: from dchagin.static.corbina.net (dchagin.static.corbina.ru. [78.107.232.239]) by mx.google.com with ESMTPSA id f9sm20645168lae.16.2014.05.19.12.02.56 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 May 2014 12:02:56 -0700 (PDT) Sender: Dmitry Chagin Received: from dchagin.static.corbina.net (localhost [127.0.0.1]) by dchagin.static.corbina.net (8.14.8/8.14.8) with ESMTP id s4JJ2sKD098736; Mon, 19 May 2014 23:02:54 +0400 (MSK) (envelope-from dchagin@dchagin.static.corbina.net) Received: (from dchagin@localhost) by dchagin.static.corbina.net (8.14.8/8.14.8/Submit) id s4JJ2sMY098735; Mon, 19 May 2014 23:02:54 +0400 (MSK) (envelope-from dchagin) Date: Mon, 19 May 2014 23:02:54 +0400 From: Chagin Dmitry To: Jilles Tjoelker 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> References: <201405171439.s4HEdedl051853@svn.freebsd.org> <20140518124453.GA3555@stack.nl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="xHFwDpU9dbj6ez1V" Content-Disposition: inline In-Reply-To: <20140518124453.GA3555@stack.nl> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: src-committers@freebsd.org, svn-src-user@freebsd.org X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 May 2014 19:03:00 -0000 --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--