From owner-svn-src-head@FreeBSD.ORG Fri May 2 18:37:13 2014 Return-Path: Delivered-To: svn-src-head@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 55BBBB15; Fri, 2 May 2014 18:37:13 +0000 (UTC) Received: from mail-lb0-x231.google.com (mail-lb0-x231.google.com [IPv6:2a00:1450:4010:c04::231]) (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 3AFC71BDA; Fri, 2 May 2014 18:37:12 +0000 (UTC) Received: by mail-lb0-f177.google.com with SMTP id z11so3431595lbi.22 for ; Fri, 02 May 2014 11:37:09 -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=DeObnN1NZmM/VojyG+tGKCfJL9kX1PrC61dkKUMV1xA=; b=VGxJ3hN7XrOV/4wOS/sWvmfv9bUHiJ4Y3pfpYG/1Kthx0lRflqNb+T1eXkeMlSli3/ TGKVEB/RxBUaPd9on33y1DFNflhO4kaXYIghNJuM8wXAXqZS8qsZ1OEJpWtSl4xjQzAx 0u7t3lGU63rPMLLKtdF2JXrcI0YzIf5BFW9ioLnPgfBOZ77RnMO7xBYtoXfwiONk/ZDH DwzEcR+mw+YKYT/c645rEy8Vea2Xiwm+MA35RnH0UNHkMuk1hJ1Y+4uBgJpxNX1m1f2W zGMX+0jmHBYjnkddctY/PrHS9qpv0auepxd1achZ4zojWDLwLRur+QUTK6hhawewk2EK t/ig== X-Received: by 10.112.135.198 with SMTP id pu6mr1190571lbb.58.1399055829791; Fri, 02 May 2014 11:37:09 -0700 (PDT) Received: from dchagin.static.corbina.net (dchagin.static.corbina.ru. [78.107.232.239]) by mx.google.com with ESMTPSA id rd5sm34555506lbb.0.2014.05.02.11.37.07 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 May 2014 11:37:07 -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 s42Ib6JG010318; Fri, 2 May 2014 22:37:06 +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 s42Ib5jQ010317; Fri, 2 May 2014 22:37:05 +0400 (MSK) (envelope-from dchagin) Date: Fri, 2 May 2014 22:37:05 +0400 From: Chagin Dmitry To: Mateusz Guzik Subject: Re: svn commit: r255672 - in head/sys: amd64/linux32 compat/linux conf i386/linux kern modules/linux sys Message-ID: <20140502183705.GA10245@dchagin.static.corbina.net> References: <201309181756.r8IHu4qV052882@svn.freebsd.org> <20130918184648.GA31748@dft-labs.eu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="X1bOJ3K7DJ5YkBrT" Content-Disposition: inline In-Reply-To: <20130918184648.GA31748@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: svn-src-head@freebsd.org, yuri@rawbw.com, Roman Divacky , src-committers@freebsd.org, svn-src-all@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 May 2014 18:37:13 -0000 --X1bOJ3K7DJ5YkBrT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 18, 2013 at 08:46:48PM +0200, Mateusz Guzik wrote: > On Wed, Sep 18, 2013 at 05:56:04PM +0000, Roman Divacky wrote: > > Author: rdivacky > > Date: Wed Sep 18 17:56:04 2013 > > New Revision: 255672 > > URL: http://svnweb.freebsd.org/changeset/base/255672 > >=20 > > Log: > > Implement epoll support in Linuxulator. This is a tiny wrapper around= kqueue > > to implement epoll subset of functionality. The kqueue user data are = 32bit > > on i386 which is not enough for epoll user data so this patch overrid= es > > kqueue fileops to maintain enough space in struct file. > > =20 > > Initial patch developed by me in 2007 and then extended and finished > > by Yuri Victorovich. > > =20 >=20 I'm strongly dislike a hacking kqueue file ops. I would prefer more generic mechanism. Maybe we need a emulator file ops in struct file, or a per proc list of such files. Any ideas? > First of all thank you both for doing this work. >=20 > I have some important though (I didn't spend too much on this, so maybe > I missed some stuff or what I write here is incorrect). >=20 > In general some lines are longer than 80 characters and simple stile > violations are present ("if (!error)"). >=20 > > +/* Create a new epoll file descriptor. */ > > + > > +static int > > +linux_epoll_create_common(struct thread *td) > > +{ > > + struct file *fp; > > + int error; > > + > > + error =3D kern_kqueue_locked(td, &fp); > > +#if EPOLL_WIDE_USER_DATA > > + if (error =3D=3D 0) { > > + epoll_init_user_data(td, fp); > > + fdrop(fp, td); > > + } > > +#endif > > + return (error); > > +} >=20 > This leaks fd reference if EPOLL_WIDE_USER_DATA is not defined. >=20 > > +int > > +linux_epoll_create1(struct thread *td, struct linux_epoll_create1_args= *args) > > +{ > > + int error; > > + > > + error =3D linux_epoll_create_common(td); > > + > > + if (!error) { > > + if (args->flags & LINUX_EPOLL_CLOEXEC) > > + td->td_proc->p_fd->fd_ofiles[td->td_retval[0]].fde_flags |=3D UF_EX= CLOSE; >=20 > This is very racy for no good reason. This should be passed down to > kqueue and be set on fd creation. >=20 > > + if (args->flags & LINUX_EPOLL_NONBLOCK) > > + linux_msg(td, "epoll_create1 doesn't yet support EPOLL_NONBLOCK fla= g\n"); > > + } > > + > > + return (error); > > +} > > + > > + > > +static void > > +epoll_init_user_data(struct thread *td, struct file *epfp) > > +{ > > + struct epoll_user_data *udv; > > + > > + /* override file ops to have our close operation */ > > + atomic_store_rel_ptr((volatile uintptr_t *)&epfp->f_ops, (uintptr_t)&= epollops); > > + > > + /* allocate epoll_user_data initially for up to 16 file descriptor va= lues */ > > + udv =3D malloc(EPOLL_USER_DATA_SIZE(EPOLL_USER_DATA_MARGIN), M_LINUX_= EPOLL, M_WAITOK); > > + udv->sz =3D EPOLL_USER_DATA_MARGIN; > > + EPOLL_USER_DATA_SET(epfp, udv); > > +} >=20 > Is not this racy? There is a window when fd is installed with epoll ops, > yet no userdata is allocated. >=20 > > +/*ARGSUSED*/ > > +static int > > +epoll_close(struct file *epfp, struct thread *td) > > +{ > > + /* free user data vector */ > > + free(EPOLL_USER_DATA_GET(epfp), M_LINUX_EPOLL); > > + /* over to kqueue parent */ > > + return (kqueue_close(epfp, td)); > > +} > > +#endif >=20 > Unnecessary comments. >=20 > > + > > +static struct file* > > +epoll_fget(struct thread *td, int epfd) > > +{ > > + struct file *fp; > > + cap_rights_t rights; > > + > > + if (fget(td, epfd, cap_rights_init(&rights, CAP_POLL_EVENT), &fp) != =3D 0) > > + panic("epoll: no file object found for kqueue descriptor"); > > + > > + return (fp); > > +} > > + >=20 > Callers pass arbitrary fd here (provided by the user), yet this panics > if fd is bad. >=20 > > int > > +kern_kqueue(struct thread *td) > > +{ > > + struct file *fp; > > + int error; > > + > > + error =3D kern_kqueue_locked(td, &fp); > > + >=20 > Why is this _locked? Typically such naming is used when some locks are > held around the call. >=20 > > + fdrop(fp, td); >=20 > If there was an error, fdrop is called even though there is nothing to > fdrop. >=20 > > + return (error); > > +} >=20 >=20 > --=20 > Mateusz Guzik --=20 Have fun! chd --X1bOJ3K7DJ5YkBrT Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlNj5dEACgkQ0t2Tb3OO/O3BggCeLOaWC/Sahncjp9L2+Vls9oPS F8oAoM6ssx7iJhs8gFX+14v2yM0HJA2P =igRs -----END PGP SIGNATURE----- --X1bOJ3K7DJ5YkBrT--