Date: Fri, 2 May 2014 22:37:05 +0400 From: Chagin Dmitry <dchagin@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, yuri@rawbw.com, Roman Divacky <rdivacky@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org 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> In-Reply-To: <20130918184648.GA31748@dft-labs.eu> References: <201309181756.r8IHu4qV052882@svn.freebsd.org> <20130918184648.GA31748@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <mjguzik gmail.com> --=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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140502183705.GA10245>