svn commit: r255672 - in head/sys: amd64/linux32 compat/linux conf i386/linux kern modules/linux sys
Chagin Dmitry
dchagin at freebsd.org
Fri May 2 18:37:13 UTC 2014
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
> >
> > 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 overrides
> > kqueue fileops to maintain enough space in struct file.
> >
> > Initial patch developed by me in 2007 and then extended and finished
> > by Yuri Victorovich.
> >
>
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.
>
> 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).
>
> In general some lines are longer than 80 characters and simple stile
> violations are present ("if (!error)").
>
> > +/* Create a new epoll file descriptor. */
> > +
> > +static int
> > +linux_epoll_create_common(struct thread *td)
> > +{
> > + struct file *fp;
> > + int error;
> > +
> > + error = kern_kqueue_locked(td, &fp);
> > +#if EPOLL_WIDE_USER_DATA
> > + if (error == 0) {
> > + epoll_init_user_data(td, fp);
> > + fdrop(fp, td);
> > + }
> > +#endif
> > + return (error);
> > +}
>
> This leaks fd reference if EPOLL_WIDE_USER_DATA is not defined.
>
> > +int
> > +linux_epoll_create1(struct thread *td, struct linux_epoll_create1_args *args)
> > +{
> > + int error;
> > +
> > + error = 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 |= UF_EXCLOSE;
>
> This is very racy for no good reason. This should be passed down to
> kqueue and be set on fd creation.
>
> > + if (args->flags & LINUX_EPOLL_NONBLOCK)
> > + linux_msg(td, "epoll_create1 doesn't yet support EPOLL_NONBLOCK flag\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 values */
> > + udv = malloc(EPOLL_USER_DATA_SIZE(EPOLL_USER_DATA_MARGIN), M_LINUX_EPOLL, M_WAITOK);
> > + udv->sz = EPOLL_USER_DATA_MARGIN;
> > + EPOLL_USER_DATA_SET(epfp, udv);
> > +}
>
> Is not this racy? There is a window when fd is installed with epoll ops,
> yet no userdata is allocated.
>
> > +/*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
>
> Unnecessary comments.
>
> > +
> > +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) != 0)
> > + panic("epoll: no file object found for kqueue descriptor");
> > +
> > + return (fp);
> > +}
> > +
>
> Callers pass arbitrary fd here (provided by the user), yet this panics
> if fd is bad.
>
> > int
> > +kern_kqueue(struct thread *td)
> > +{
> > + struct file *fp;
> > + int error;
> > +
> > + error = kern_kqueue_locked(td, &fp);
> > +
>
> Why is this _locked? Typically such naming is used when some locks are
> held around the call.
>
> > + fdrop(fp, td);
>
> If there was an error, fdrop is called even though there is nothing to
> fdrop.
>
> > + return (error);
> > +}
>
>
> --
> Mateusz Guzik <mjguzik gmail.com>
--
Have fun!
chd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140502/901ad58e/attachment.sig>
More information about the svn-src-head
mailing list