From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Fernandes Subject: Re: [RFC v2 1/3] dma-buf: give each buffer a full-fledged inode Date: Mon, 25 Mar 2019 17:47:38 -0400 Message-ID: <20190325214738.GB16969@google.com> References: <20190322025135.118201-1-fengc@google.com> <20190322025135.118201-2-fengc@google.com> <20190322150255.GA76423@google.com> <20190324175633.GA5826@google.com> <20190324204454.GA102207@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Chenbo Feng Cc: Sandeep Patil , LKML , DRI mailing list , linux-media@vger.kernel.org, kernel-team@android.com, Sumit Semwal , Erick Reyes , Daniel Vetter , John Stultz List-Id: dri-devel@lists.freedesktop.org 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//fd and /proc//maps. I guess > > > the map_files bit is for consistency? > > > > It just occured to me that since /proc/ > 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/ 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//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