| Message ID | 1529660407-6266-1-git-send-email-akhilpo@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akhil, On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > Each fence object holds function pointers of the module that > initialized > it. Allowing the module to unload before this fence's release is > catastrophic. So, keep a refcount on the module until the fence is > released. > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > --- > Changes in v2: > - added description for the new function parameter. > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > include/linux/dma-fence.h | 10 ++++++++-- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 4edb9fd..2aaa44e 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -18,6 +18,7 @@ > * more details. > */ > > +#include <linux/module.h> > #include <linux/slab.h> > #include <linux/export.h> > #include <linux/atomic.h> > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > { > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > + struct module *module = fence->owner; > > trace_dma_fence_destroy(fence); > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > fence->ops->release(fence); > else > dma_fence_free(fence); > + > + module_put(module); > } > EXPORT_SYMBOL(dma_fence_release); > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > /** > * dma_fence_init - Initialize a custom fence. > + * @module: [in] the module that calls this API > * @fence: [in] the fence to initialize > * @ops: [in] the dma_fence_ops for operations on this > fence > * @lock: [in] the irqsafe spinlock to use for locking > this fence > @@ -556,8 +561,9 @@ struct default_wait_cb { > * to check which fence is later by simply using dma_fence_later. > */ > void > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > *ops, > - spinlock_t *lock, u64 context, unsigned seqno) > +_dma_fence_init(struct module *module, struct dma_fence *fence, > + const struct dma_fence_ops *ops, spinlock_t *lock, > + u64 context, unsigned seqno) > { > BUG_ON(!lock); > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > @@ -571,7 +577,11 @@ struct default_wait_cb { > fence->seqno = seqno; > fence->flags = 0UL; > fence->error = 0; > + fence->owner = module; > + > + if (!try_module_get(module)) > + fence->owner = NULL; > > trace_dma_fence_init(fence); > } > -EXPORT_SYMBOL(dma_fence_init); > +EXPORT_SYMBOL(_dma_fence_init); Do we still need to export the symbol, it won't be called from outside anymore? Other than that looks good to me: Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Gustavo
Quoting Gustavo Padovan (2018-06-22 11:04:16) > Hi Akhil, > > On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > > Each fence object holds function pointers of the module that > > initialized > > it. Allowing the module to unload before this fence's release is > > catastrophic. So, keep a refcount on the module until the fence is > > released. > > > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > > --- > > Changes in v2: > > - added description for the new function parameter. > > > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > > include/linux/dma-fence.h | 10 ++++++++-- > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > fence.c > > index 4edb9fd..2aaa44e 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -18,6 +18,7 @@ > > * more details. > > */ > > > > +#include <linux/module.h> > > #include <linux/slab.h> > > #include <linux/export.h> > > #include <linux/atomic.h> > > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > > { > > struct dma_fence *fence = > > container_of(kref, struct dma_fence, refcount); > > + struct module *module = fence->owner; > > > > trace_dma_fence_destroy(fence); > > > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > > fence->ops->release(fence); > > else > > dma_fence_free(fence); > > + > > + module_put(module); > > } > > EXPORT_SYMBOL(dma_fence_release); > > > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > > > /** > > * dma_fence_init - Initialize a custom fence. > > + * @module: [in] the module that calls this API > > * @fence: [in] the fence to initialize > > * @ops: [in] the dma_fence_ops for operations on this > > fence > > * @lock: [in] the irqsafe spinlock to use for locking > > this fence > > @@ -556,8 +561,9 @@ struct default_wait_cb { > > * to check which fence is later by simply using dma_fence_later. > > */ > > void > > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > > *ops, > > - spinlock_t *lock, u64 context, unsigned seqno) > > +_dma_fence_init(struct module *module, struct dma_fence *fence, > > + const struct dma_fence_ops *ops, spinlock_t *lock, > > + u64 context, unsigned seqno) > > { > > BUG_ON(!lock); > > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > > @@ -571,7 +577,11 @@ struct default_wait_cb { > > fence->seqno = seqno; > > fence->flags = 0UL; > > fence->error = 0; > > + fence->owner = module; > > + > > + if (!try_module_get(module)) > > + fence->owner = NULL; > > > > trace_dma_fence_init(fence); > > } > > -EXPORT_SYMBOL(dma_fence_init); > > +EXPORT_SYMBOL(_dma_fence_init); > > Do we still need to export the symbol, it won't be called from outside > anymore? Other than that looks good to me: There's a big drawback in that a module reference is often insufficient, and that a reference on the driver (or whatever is required for the lifetime of the fence) will already hold the module reference. Considering that we want a few 100k fences in flight per second, is there no other way to only export a fence with a module reference? -Chris
On 6/22/2018 3:38 PM, Chris Wilson wrote: > Quoting Gustavo Padovan (2018-06-22 11:04:16) >> Hi Akhil, >> >> On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: >>> Each fence object holds function pointers of the module that >>> initialized >>> it. Allowing the module to unload before this fence's release is >>> catastrophic. So, keep a refcount on the module until the fence is >>> released. >>> >>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>> --- >>> Changes in v2: >>> - added description for the new function parameter. >>> >>> drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- >>> include/linux/dma-fence.h | 10 ++++++++-- >>> 2 files changed, 21 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- >>> fence.c >>> index 4edb9fd..2aaa44e 100644 >>> --- a/drivers/dma-buf/dma-fence.c >>> +++ b/drivers/dma-buf/dma-fence.c >>> @@ -18,6 +18,7 @@ >>> * more details. >>> */ >>> >>> +#include <linux/module.h> >>> #include <linux/slab.h> >>> #include <linux/export.h> >>> #include <linux/atomic.h> >>> @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) >>> { >>> struct dma_fence *fence = >>> container_of(kref, struct dma_fence, refcount); >>> + struct module *module = fence->owner; >>> >>> trace_dma_fence_destroy(fence); >>> >>> @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) >>> fence->ops->release(fence); >>> else >>> dma_fence_free(fence); >>> + >>> + module_put(module); >>> } >>> EXPORT_SYMBOL(dma_fence_release); >>> >>> @@ -541,6 +545,7 @@ struct default_wait_cb { >>> >>> /** >>> * dma_fence_init - Initialize a custom fence. >>> + * @module: [in] the module that calls this API >>> * @fence: [in] the fence to initialize >>> * @ops: [in] the dma_fence_ops for operations on this >>> fence >>> * @lock: [in] the irqsafe spinlock to use for locking >>> this fence >>> @@ -556,8 +561,9 @@ struct default_wait_cb { >>> * to check which fence is later by simply using dma_fence_later. >>> */ >>> void >>> -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops >>> *ops, >>> - spinlock_t *lock, u64 context, unsigned seqno) >>> +_dma_fence_init(struct module *module, struct dma_fence *fence, >>> + const struct dma_fence_ops *ops, spinlock_t *lock, >>> + u64 context, unsigned seqno) >>> { >>> BUG_ON(!lock); >>> BUG_ON(!ops || !ops->wait || !ops->enable_signaling || >>> @@ -571,7 +577,11 @@ struct default_wait_cb { >>> fence->seqno = seqno; >>> fence->flags = 0UL; >>> fence->error = 0; >>> + fence->owner = module; >>> + >>> + if (!try_module_get(module)) >>> + fence->owner = NULL; >>> >>> trace_dma_fence_init(fence); >>> } >>> -EXPORT_SYMBOL(dma_fence_init); >>> +EXPORT_SYMBOL(_dma_fence_init); >> Do we still need to export the symbol, it won't be called from outside >> anymore? Other than that looks good to me: Yes. Because dma_fence_init() is now a macro that resolves to _dma_fence_init(). > There's a big drawback in that a module reference is often insufficient, > and that a reference on the driver (or whatever is required for the > lifetime of the fence) will already hold the module reference. I didn't quite understand what you meant here. Could you please elaborate? > > Considering that we want a few 100k fences in flight per second, is > there no other way to only export a fence with a module reference? > -Chris Thanks, Akhil.
On Fri, Jun 22, 2018 at 11:08:48AM +0100, Chris Wilson wrote: > Quoting Gustavo Padovan (2018-06-22 11:04:16) > > Hi Akhil, > > > > On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > > > Each fence object holds function pointers of the module that > > > initialized > > > it. Allowing the module to unload before this fence's release is > > > catastrophic. So, keep a refcount on the module until the fence is > > > released. > > > > > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > > > --- > > > Changes in v2: > > > - added description for the new function parameter. > > > > > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > > > include/linux/dma-fence.h | 10 ++++++++-- > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > fence.c > > > index 4edb9fd..2aaa44e 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -18,6 +18,7 @@ > > > * more details. > > > */ > > > > > > +#include <linux/module.h> > > > #include <linux/slab.h> > > > #include <linux/export.h> > > > #include <linux/atomic.h> > > > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > > > { > > > struct dma_fence *fence = > > > container_of(kref, struct dma_fence, refcount); > > > + struct module *module = fence->owner; > > > > > > trace_dma_fence_destroy(fence); > > > > > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > > > fence->ops->release(fence); > > > else > > > dma_fence_free(fence); > > > + > > > + module_put(module); > > > } > > > EXPORT_SYMBOL(dma_fence_release); > > > > > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > > > > > /** > > > * dma_fence_init - Initialize a custom fence. > > > + * @module: [in] the module that calls this API > > > * @fence: [in] the fence to initialize > > > * @ops: [in] the dma_fence_ops for operations on this > > > fence > > > * @lock: [in] the irqsafe spinlock to use for locking > > > this fence > > > @@ -556,8 +561,9 @@ struct default_wait_cb { > > > * to check which fence is later by simply using dma_fence_later. > > > */ > > > void > > > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > > > *ops, > > > - spinlock_t *lock, u64 context, unsigned seqno) > > > +_dma_fence_init(struct module *module, struct dma_fence *fence, > > > + const struct dma_fence_ops *ops, spinlock_t *lock, > > > + u64 context, unsigned seqno) > > > { > > > BUG_ON(!lock); > > > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > > > @@ -571,7 +577,11 @@ struct default_wait_cb { > > > fence->seqno = seqno; > > > fence->flags = 0UL; > > > fence->error = 0; > > > + fence->owner = module; > > > + > > > + if (!try_module_get(module)) > > > + fence->owner = NULL; > > > > > > trace_dma_fence_init(fence); > > > } > > > -EXPORT_SYMBOL(dma_fence_init); > > > +EXPORT_SYMBOL(_dma_fence_init); > > > > Do we still need to export the symbol, it won't be called from outside > > anymore? Other than that looks good to me: > > There's a big drawback in that a module reference is often insufficient, > and that a reference on the driver (or whatever is required for the > lifetime of the fence) will already hold the module reference. > > Considering that we want a few 100k fences in flight per second, is > there no other way to only export a fence with a module reference? We'd need to make the timeline a full-blown object (Maarten owes me one for that design screw-up), and then we could stuff all these things in there. And I think that's the right fix, since try_module_get for every dma_fence_init just ain't cool really :-) -Daniel
On Mon, Jun 25, 2018 at 09:21:15PM +0530, Akhil P Oommen wrote: > > > On 6/25/2018 1:20 PM, Daniel Vetter wrote: > > On Fri, Jun 22, 2018 at 11:08:48AM +0100, Chris Wilson wrote: > > > Quoting Gustavo Padovan (2018-06-22 11:04:16) > > > > Hi Akhil, > > > > > > > > On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > > > > > Each fence object holds function pointers of the module that > > > > > initialized > > > > > it. Allowing the module to unload before this fence's release is > > > > > catastrophic. So, keep a refcount on the module until the fence is > > > > > released. > > > > > > > > > > Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> > > > > > --- > > > > > Changes in v2: > > > > > - added description for the new function parameter. > > > > > > > > > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > > > > > include/linux/dma-fence.h | 10 ++++++++-- > > > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > > > fence.c > > > > > index 4edb9fd..2aaa44e 100644 > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > @@ -18,6 +18,7 @@ > > > > > * more details. > > > > > */ > > > > > +#include <linux/module.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/export.h> > > > > > #include <linux/atomic.h> > > > > > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > > > > > { > > > > > struct dma_fence *fence = > > > > > container_of(kref, struct dma_fence, refcount); > > > > > + struct module *module = fence->owner; > > > > > trace_dma_fence_destroy(fence); > > > > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > > > > > fence->ops->release(fence); > > > > > else > > > > > dma_fence_free(fence); > > > > > + > > > > > + module_put(module); > > > > > } > > > > > EXPORT_SYMBOL(dma_fence_release); > > > > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > > > > /** > > > > > * dma_fence_init - Initialize a custom fence. > > > > > + * @module: [in] the module that calls this API > > > > > * @fence: [in] the fence to initialize > > > > > * @ops: [in] the dma_fence_ops for operations on this > > > > > fence > > > > > * @lock: [in] the irqsafe spinlock to use for locking > > > > > this fence > > > > > @@ -556,8 +561,9 @@ struct default_wait_cb { > > > > > * to check which fence is later by simply using dma_fence_later. > > > > > */ > > > > > void > > > > > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > > > > > *ops, > > > > > - spinlock_t *lock, u64 context, unsigned seqno) > > > > > +_dma_fence_init(struct module *module, struct dma_fence *fence, > > > > > + const struct dma_fence_ops *ops, spinlock_t *lock, > > > > > + u64 context, unsigned seqno) > > > > > { > > > > > BUG_ON(!lock); > > > > > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > > > > > @@ -571,7 +577,11 @@ struct default_wait_cb { > > > > > fence->seqno = seqno; > > > > > fence->flags = 0UL; > > > > > fence->error = 0; > > > > > + fence->owner = module; > > > > > + > > > > > + if (!try_module_get(module)) > > > > > + fence->owner = NULL; > > > > > trace_dma_fence_init(fence); > > > > > } > > > > > -EXPORT_SYMBOL(dma_fence_init); > > > > > +EXPORT_SYMBOL(_dma_fence_init); > > > > Do we still need to export the symbol, it won't be called from outside > > > > anymore? Other than that looks good to me: > > > There's a big drawback in that a module reference is often insufficient, > > > and that a reference on the driver (or whatever is required for the > > > lifetime of the fence) will already hold the module reference. > > > > > > Considering that we want a few 100k fences in flight per second, is > > > there no other way to only export a fence with a module reference? > > We'd need to make the timeline a full-blown object (Maarten owes me one > > for that design screw-up), and then we could stuff all these things in > > there. > > > > And I think that's the right fix, since try_module_get for every > > dma_fence_init just ain't cool really :-) > > -Daniel > Thanks for the feedback, Daniel. > I see your point, but I am not sure how much impact an extra refcounting > would create considering the whole effort of setting up a new fence. Also, > this refcounting is not required for built-in modules. > > As of now, unloading a kernel module that uses fence_init() is an easy way > to bring down the system. This patch simply fixes that. What you have > suggested sounds like a non-trivial effort which someone who is more > familiar with this code base can do a better job than me. Perhaps we can > take this patch now to fix the issue at hand and later somebody else can > share a more optimal solution. :) Module unload is a developer-only feature for a reason. Given that I don't think fixing this with a hack is the right approach. And dma_fence_init() is supposed to be really fast. Also note that you can fix this already for your own driver by simply waiting for all pending dma_fences to get released, so I don't think it's super-important to land this asap. Yes the real fix is a bit more involved, but shouldn't be too hard to pull off really. -Daniel > > @Gustavo & @Sumit, I would like the maintainers to take a decision here. > > -Akhil. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 4edb9fd..2aaa44e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -18,6 +18,7 @@ * more details. */ +#include <linux/module.h> #include <linux/slab.h> #include <linux/export.h> #include <linux/atomic.h> @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) { struct dma_fence *fence = container_of(kref, struct dma_fence, refcount); + struct module *module = fence->owner; trace_dma_fence_destroy(fence); @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) fence->ops->release(fence); else dma_fence_free(fence); + + module_put(module); } EXPORT_SYMBOL(dma_fence_release); @@ -541,6 +545,7 @@ struct default_wait_cb { /** * dma_fence_init - Initialize a custom fence. + * @module: [in] the module that calls this API * @fence: [in] the fence to initialize * @ops: [in] the dma_fence_ops for operations on this fence * @lock: [in] the irqsafe spinlock to use for locking this fence @@ -556,8 +561,9 @@ struct default_wait_cb { * to check which fence is later by simply using dma_fence_later. */ void -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, - spinlock_t *lock, u64 context, unsigned seqno) +_dma_fence_init(struct module *module, struct dma_fence *fence, + const struct dma_fence_ops *ops, spinlock_t *lock, + u64 context, unsigned seqno) { BUG_ON(!lock); BUG_ON(!ops || !ops->wait || !ops->enable_signaling || @@ -571,7 +577,11 @@ struct default_wait_cb { fence->seqno = seqno; fence->flags = 0UL; fence->error = 0; + fence->owner = module; + + if (!try_module_get(module)) + fence->owner = NULL; trace_dma_fence_init(fence); } -EXPORT_SYMBOL(dma_fence_init); +EXPORT_SYMBOL(_dma_fence_init); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index eb9b05a..8159125 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -36,6 +36,8 @@ /** * struct dma_fence - software synchronization primitive + * @owner: the module that contains fence_ops functions. + * Usually THIS_MODULE. * @refcount: refcount for this fence * @ops: dma_fence_ops associated with this fence * @rcu: used for releasing fence with kfree_rcu @@ -71,6 +73,7 @@ * been completed, or never called at all. */ struct dma_fence { + struct module *owner; struct kref refcount; const struct dma_fence_ops *ops; struct rcu_head rcu; @@ -249,8 +252,11 @@ struct dma_fence_ops { char *str, int size); }; -void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, - spinlock_t *lock, u64 context, unsigned seqno); +#define dma_fence_init(fence, ops, lock, context, seqno) _dma_fence_init( \ + THIS_MODULE, fence, ops, lock, context, seqno) +void _dma_fence_init(struct module *module, struct dma_fence *fence, + const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, + unsigned seqno); void dma_fence_release(struct kref *kref); void dma_fence_free(struct dma_fence *fence);
Each fence object holds function pointers of the module that initialized it. Allowing the module to unload before this fence's release is catastrophic. So, keep a refcount on the module until the fence is released. Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> --- Changes in v2: - added description for the new function parameter. drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- include/linux/dma-fence.h | 10 ++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-)