[PATCH RFC RESEND] dma-buf/fs Add get_[file|dma_buf]_unless_doomed

[PATCH RFC RESEND] dma-buf/fs Add get_[file|dma_buf]_unless_doomed

[Thomas Hellstrom]

Resending since it appears this RFC never got to the dri-devel lkml lists.

In this context, a "doomed" object is an object whose refcount has reached
zero, but that has not yet been freed.

To avoid mutual refcounting vmwgfx need to have a non-refcounted pointer to
a dma-buf in a lookup structure. The pointer is removed in the dma-buf
destructor. To allow lookup-structure private locks we need
get_dma_buf_unless_doomed(). This common refcounting scenario is described
with examples in detail in the kref documentaion.
The solution with local locks is under kref_get_unless_zero().
See also kobject_get_unless_zero() and its commit message.
Since dma-bufs are using the attached file for refcounting,
get_dma_buf_unless_doomed maps directly to a get_file_unless_doomed.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 include/linux/dma-buf.h |   16 ++++++++++++++++
 include/linux/fs.h      |   15 +++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index dfac5ed..6e58144 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -162,6 +162,22 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
 	get_file(dmabuf->file);
 }
 
+/**
+ * get_dma_buf_unless_doomed - convenience wrapper for get_file_unless_doomed
+ *
+ * @dmabuf:	[in]	pointer to dma_buf
+ *
+ * Obtain a dma-buf reference from a lookup structure that doesn't refcount
+ * the dma-buf, but synchronizes with its release method to make sure it has
+ * not been freed yet. See for example kref_get_unless_zero documentation.
+ * Returns true if refcounting succeeds, false otherwise.
+ */
+static inline bool __must_check
+get_dma_buf_unless_doomed(struct dma_buf *dmabuf)
+{
+	return get_file_unless_doomed(dmabuf->file);
+}
+
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 							struct device *dev);
 void dma_buf_detach(struct dma_buf *dmabuf,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f40547..a96c333 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -824,6 +824,21 @@ static inline struct file *get_file(struct file *f)
 	atomic_long_inc(&f->f_count);
 	return f;
 }
+
+/**
+ * get_file_unless_doomed - convenience wrapper for get_file_unless_doomed
+ * @file:	[in]	pointer to file
+ *
+ * Obtain a file reference from a lookup structure that doesn't refcount
+ * the file, but synchronizes with its release method to make sure it has
+ * not been freed yet. See for example kref_get_unless_zero documentation.
+ * Returns true if refcounting succeeds, false otherwise.
+ */
+static inline bool __must_check get_file_unless_doomed(struct file *f)
+{
+	return atomic_long_inc_not_zero(&f->f_count) != 0L;
+}
+
 #define fput_atomic(x)	atomic_long_add_unless(&(x)->f_count, -1, 1)
 #define file_count(x)	atomic_long_read(&(x)->f_count)
 
-- 
1.7.10.4

Re: [PATCH RFC RESEND] dma-buf/fs Add get_[file|dma_buf]_unless_doomed

[Al Viro]

On Mon, Nov 11, 2013 at 12:57:47AM -0800, Thomas Hellstrom wrote:
> Resending since it appears this RFC never got to the dri-devel lkml lists.
> 
> In this context, a "doomed" object is an object whose refcount has reached
> zero, but that has not yet been freed.
> 
> To avoid mutual refcounting vmwgfx need to have a non-refcounted pointer to
> a dma-buf in a lookup structure. The pointer is removed in the dma-buf
> destructor. To allow lookup-structure private locks we need
> get_dma_buf_unless_doomed(). This common refcounting scenario is described
> with examples in detail in the kref documentaion.
> The solution with local locks is under kref_get_unless_zero().
> See also kobject_get_unless_zero() and its commit message.
> Since dma-bufs are using the attached file for refcounting,
> get_dma_buf_unless_doomed maps directly to a get_file_unless_doomed.

NAK for struct file.  This kind of stuff is for implementing primitives,
not as a public API.  BTW, as for dmabuf...  dma_buf_fd() calling conventions
are seriously misguided - we are trying to transfer the reference we hold
into something (in this case - descriptor table), so the failure exit
should be dropping the reference, not leaving that to caller.

Re: [PATCH RFC RESEND] dma-buf/fs Add get_[file|dma_buf]_unless_doomed

[Thomas Hellstrom]

On 11/11/2013 04:17 PM, Al Viro wrote:
> On Mon, Nov 11, 2013 at 12:57:47AM -0800, Thomas Hellstrom wrote:
>> Resending since it appears this RFC never got to the dri-devel lkml lists.
>>
>> In this context, a "doomed" object is an object whose refcount has reached
>> zero, but that has not yet been freed.
>>
>> To avoid mutual refcounting vmwgfx need to have a non-refcounted pointer to
>> a dma-buf in a lookup structure. The pointer is removed in the dma-buf
>> destructor. To allow lookup-structure private locks we need
>> get_dma_buf_unless_doomed(). This common refcounting scenario is described
>> with examples in detail in the kref documentaion.
>> The solution with local locks is under kref_get_unless_zero().
>> See also kobject_get_unless_zero() and its commit message.
>> Since dma-bufs are using the attached file for refcounting,
>> get_dma_buf_unless_doomed maps directly to a get_file_unless_doomed.
> NAK for struct file.  This kind of stuff is for implementing primitives,
> not as a public API.

That's only partially correct. Public access (through in this case a 
release callback) to the object destructor validates a public 
ref_unless_doomed method. It's particularly useful if an external user 
wants to implement a lookup table for objects that are removed from the 
table in the object destructor or, (which doesn't apply in this case) 
using RCU locks for lookups.

Still want to NAK this, could you please elaborate a bit more why you 
don't want it in? Fear of misuse or general dislike of this
lookup method?

>   BTW, as for dmabuf...  dma_buf_fd() calling conventions
> are seriously misguided - we are trying to transfer the reference we hold
> into something (in this case - descriptor table), so the failure exit
> should be dropping the reference, not leaving that to caller

You're probably right, though I'm only a dma_buf API user, trying to 
extend the dma_buf API useful for my use-case.

Thanks,
Thomas