Message-ID: <20190325214738.GB16969@google.com>
Date:   Mon, 25 Mar 2019 17:47:38 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Chenbo Feng <fengc@...gle.com>
Cc:     Sandeep Patil <sspatil@...roid.com>,
        LKML <linux-kernel@...r.kernel.org>,
        DRI mailing list <dri-devel@...ts.freedesktop.org>,
        linux-media@...r.kernel.org, kernel-team@...roid.com,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Erick Reyes <erickreyes@...gle.com>,
        Daniel Vetter <daniel@...ll.ch>,
        John Stultz <john.stultz@...aro.org>
Subject: Re: [RFC v2 1/3] dma-buf: give each buffer a full-fledged inode

On Mon, Mar 25, 2019 at 12:34:59PM -0700, Chenbo Feng wrote:
[snip]
> > > > Also what is the benefit of having st_blocks from stat? AFAIK, that is the
> > > > same as the buffer's size which does not change for the lifetime of the
> > > > buffer. In your patch you're doing this when 'struct file' is created which
> > > > AIUI is what reflects in the st_blocks:
> > > > +   inode_set_bytes(inode, dmabuf->size);
> > >
> > > Can some of the use cases / data be trimmed down? I think so. For example, I
> > > never understood what we do with map_files here (or why). It is perfectly
> > > fine to just get the data from /proc/<pid>/fd and /proc/<pid>/maps. I guess
> > > the map_files bit is for consistency?
> >
> > It just occured to me that since /proc/<pid/maps provides an inode number as
> > one of the fields, so indeed an inode per buf is a very good idea for the
> > user to distinguish buffers just by reading /proc/<pid/<maps> alone..
> >
> > I also, similar to you, don't think map_files is useful for this usecase.
> > map_files are just symlinks named as memory ranges and pointing to a file. In
> > this case the symlink will point to default name "dmabuf" that doesn't exist.
> > If one does stat(2) on a map_file link, then it just returns the inode number
> > of the symlink, not what the map_file is pointing to, which seems kind of
> > useless.
> >
> I might be wrong but I don't think we did anything special for the
> map_files in this patch. I think the confusion might be from commit
> message where Greg mentioned the map_files to describe the behavior of
> shmem buffer when comparing it with dma-buf. The file system
> implementation and the file allocation action in this patch are just
> some minimal effort to associate each dma_buf object with an inode and
> properly populate the size information in the file object. And we
> didn't use proc/pid/map_files at all in the android implementation
> indeed.

You are right.

> > > > I am not against adding of inode per buffer, but I think we should have this
> > > > debate and make the right design choice here for what we really need.
> > >
> > > sure.
> >
> > Right, so just to summarize:
> > - The intention here is /proc/<pid>/maps will give uniqueness (via the inode
> >   number) between different memory ranges. That I think is the main benefit
> >   of the patch.
> > - stat gives the size of buffer as does fdinfo
> > - fdinfo is useful to get the reference count of number of sharers of the
> >   buffer.
> > - map_files is not that useful for this usecase but can be made useful if
> >   we can name the underlying file's dentry to something other than "dmabuf".
> > - GET_NAME is not needed since fdinfo already has the SET_NAMEd name.
> >
> > Do you agree?
> >
> Thanks for summarize it, I will look into the GET_NAME/SET_NAME ioctl
> to make it more useful as you suggested above. Also, I will try to add
> some test to verify the behavior.

Sounds great, thanks!

 - Joel

Powered by blists - more mailing lists