lkml.org 
[lkml]   [2019]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object
    On Mon, Aug 12, 2019 at 02:56:15PM -0300, Jason Gunthorpe wrote:
    > On Mon, Aug 12, 2019 at 10:28:27AM -0700, Ira Weiny wrote:
    > > On Mon, Aug 12, 2019 at 10:00:40AM -0300, Jason Gunthorpe wrote:
    > > > On Fri, Aug 09, 2019 at 03:58:30PM -0700, ira.weiny@intel.com wrote:
    > > > > From: Ira Weiny <ira.weiny@intel.com>
    > > > >
    > > > > In order for MRs to be tracked against the open verbs context the ufile
    > > > > needs to have a pointer to hand to the GUP code.
    > > > >
    > > > > No references need to be taken as this should be valid for the lifetime
    > > > > of the context.
    > > > >
    > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
    > > > > drivers/infiniband/core/uverbs.h | 1 +
    > > > > drivers/infiniband/core/uverbs_main.c | 1 +
    > > > > 2 files changed, 2 insertions(+)
    > > > >
    > > > > diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
    > > > > index 1e5aeb39f774..e802ba8c67d6 100644
    > > > > +++ b/drivers/infiniband/core/uverbs.h
    > > > > @@ -163,6 +163,7 @@ struct ib_uverbs_file {
    > > > > struct page *disassociate_page;
    > > > >
    > > > > struct xarray idr;
    > > > > + struct file *sys_file; /* backpointer to system file object */
    > > > > };
    > > >
    > > > The 'struct file' has a lifetime strictly shorter than the
    > > > ib_uverbs_file, which is kref'd on its own lifetime. Having a back
    > > > pointer like this is confouding as it will be invalid for some of the
    > > > lifetime of the struct.
    > >
    > > Ah... ok. I really thought it was the other way around.
    > >
    > > __fput() should not call ib_uverbs_close() until the last reference on struct
    > > file is released... What holds references to struct ib_uverbs_file past that?
    >
    > Child fds hold onto the internal ib_uverbs_file until they are closed

    The FDs hold the struct file, don't they?

    >
    > > Perhaps I need to add this (untested)?
    > >
    > > diff --git a/drivers/infiniband/core/uverbs_main.c
    > > b/drivers/infiniband/core/uverbs_main.c
    > > index f628f9e4c09f..654e774d9cf2 100644
    > > +++ b/drivers/infiniband/core/uverbs_main.c
    > > @@ -1125,6 +1125,8 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
    > > list_del_init(&file->list);
    > > mutex_unlock(&file->device->lists_mutex);
    > >
    > > + file->sys_file = NULL;
    >
    > Now this has unlocked updates to that data.. you'd need some lock and
    > get not zero pattern

    You can't call "get" here because I'm 99% sure we only get here when struct
    file has no references left... I could be wrong. It took me a while to work
    through the reference counting so I could have missed something.

    Ira

    \
     
     \ /
      Last update: 2019-08-12 23:16    [W:2.672 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site