compat-wireless: backporting kfree_rcu()?

compat-wireless: backporting kfree_rcu()?

[Johannes Berg]

So I was looking at backporting kfree_rcu(), and came up with this:

#define kfree_rcu(data, rcuhead)                do {                    \
                void __kfree_rcu_fn(struct rcu_head *rcu_head)  \
                {                                                       \
                        void *___ptr;                                   \
                        ___ptr = container_of(rcu_head, typeof(*(data)), rcuhead);\
                        kfree(___ptr);                                  \
                }                                                       \
                call_rcu(&(data)->rcuhead, __kfree_rcu_fn);             \
        } while (0)


This works, thanks to gcc supporting nested functions, but has one major
issue: any module using call_rcu() must have an rcu_barrier() in its
module_exit() because __kfree_rcu_fn() might be called after the module
is unloaded. For kfree_rcu() this isn't needed since the function called
lives in the base kernel.

I played around with injecting a call to rcu_barrier() into module exit
by modifying module_exit() but I couldn't make the CPP do that. Anyone
else have any ideas?

Another idea I had was to insert the __kfree_rcu_fn() code into the
compat module instead, but I can't see how to do that short of using
some source post-processing scripts.

johannes

Re: compat-wireless: backporting kfree_rcu()?

[Johannes Berg]

A complete different idea:

/* a larger rcu head struct */
struct compat_rcu_head {
	struct rcu_head rh; /* must be first */
	void *free_ptr;
};

/* define kfree_rcu */
extern void compat_free_rcu(struct rcu_head *rh);

#define kfree_rcu(ptr, head)	\
	do {
		(ptr)->head.free_ptr = ptr;
		call_rcu(&(ptr)->head.rh, compat_free_rcu);
	} while (0)

/* use it in all code */
#define rcu_head compat_rcu_head


and somewhere in a compat.ko file:

#undef rcu_head
void compat_free_rcu(struct rcu_head *rh)
{
	struct compat_rcu_head *crh = (void *)rh;
	kfree(crh->free_ptr);
}


thoughts?

johannes

Re: compat-wireless: backporting kfree_rcu()?

[Johannes Berg]

On Tue, 2011-10-18 at 12:07 +0200, Johannes Berg wrote:
> A complete different idea:
> 
> /* a larger rcu head struct */
> struct compat_rcu_head {
> 	struct rcu_head rh; /* must be first */
> 	void *free_ptr;
> };
> 
> /* define kfree_rcu */
> extern void compat_free_rcu(struct rcu_head *rh);
> 
> #define kfree_rcu(ptr, head)	\
> 	do {
> 		(ptr)->head.free_ptr = ptr;
> 		call_rcu(&(ptr)->head.rh, compat_free_rcu);
> 	} while (0)
> 
> /* use it in all code */
> #define rcu_head compat_rcu_head
> 
> 
> and somewhere in a compat.ko file:
> 
> #undef rcu_head
> void compat_free_rcu(struct rcu_head *rh)
> {
> 	struct compat_rcu_head *crh = (void *)rh;
> 	kfree(crh->free_ptr);
> }
> 
> 
> thoughts?

Doesn't work either -- the new rcu_head will be included in existing
header files like dst.h and they'd get messed up if included after
compat-3.0.h

johannes

Re: compat-wireless: backporting kfree_rcu()?

[Hauke Mehrtens]

Hi Johannes,

now your patch using kfree_rcu went into linux-next. ;-)

On 10/18/2011 10:18 AM, Johannes Berg wrote:
> So I was looking at backporting kfree_rcu(), and came up with this:
> 
> #define kfree_rcu(data, rcuhead)                do {                    \
>                 void __kfree_rcu_fn(struct rcu_head *rcu_head)  \
>                 {                                                       \
>                         void *___ptr;                                   \
>                         ___ptr = container_of(rcu_head, typeof(*(data)), rcuhead);\
>                         kfree(___ptr);                                  \
>                 }                                                       \
>                 call_rcu(&(data)->rcuhead, __kfree_rcu_fn);             \
>         } while (0)
> 
> 
> This works, thanks to gcc supporting nested functions, but has one major
> issue: any module using call_rcu() must have an rcu_barrier() in its
> module_exit() because __kfree_rcu_fn() might be called after the module
> is unloaded. For kfree_rcu() this isn't needed since the function called
> lives in the base kernel.

This looks nice to me.

> I played around with injecting a call to rcu_barrier() into module exit
> by modifying module_exit() but I couldn't make the CPP do that. Anyone
> else have any ideas?

I played around with module_exit and modified the define into the
following. Now rcu_barrier() gets called on every module_exit. As
modules are not unloaded so often it should not hurd that this is also
done when unloading a module not using kfree_rcu().

#undef module_exit
#define module_exit(exitfn)					\
	void __exit __exit_rcu_barrier(void)			\
		{						\
			exitfn();				\
			rcu_barrier();				\
		}						\
	static inline exitcall_t __exittest(void)		\
	{ return exitfn; }					\
	void cleanup_module(void) __attribute__((alias("__exit_rcu_barrier")));

> 
> Another idea I had was to insert the __kfree_rcu_fn() code into the
> compat module instead, but I can't see how to do that short of using
> some source post-processing scripts.

Johannes are you fine with this?

Hauke

Re: compat-wireless: backporting kfree_rcu()?

[Johannes Berg]

Hi,

> > #define kfree_rcu(data, rcuhead)                do {                    \
> >                 void __kfree_rcu_fn(struct rcu_head *rcu_head)  \
> >                 {                                                       \
> >                         void *___ptr;                                   \
> >                         ___ptr = container_of(rcu_head, typeof(*(data)), rcuhead);\
> >                         kfree(___ptr);                                  \
> >                 }                                                       \
> >                 call_rcu(&(data)->rcuhead, __kfree_rcu_fn);             \
> >         } while (0)
> > 
> > 
> > This works, thanks to gcc supporting nested functions, but has one major
> > issue: any module using call_rcu() must have an rcu_barrier() in its
> > module_exit() because __kfree_rcu_fn() might be called after the module
> > is unloaded. For kfree_rcu() this isn't needed since the function called
> > lives in the base kernel.
> 
> This looks nice to me.

:-)

> > I played around with injecting a call to rcu_barrier() into module exit
> > by modifying module_exit() but I couldn't make the CPP do that. Anyone
> > else have any ideas?
> 
> I played around with module_exit and modified the define into the
> following. Now rcu_barrier() gets called on every module_exit. As
> modules are not unloaded so often it should not hurd that this is also
> done when unloading a module not using kfree_rcu().

Ok.

> #undef module_exit
> #define module_exit(exitfn)					\
> 	void __exit __exit_rcu_barrier(void)			\
> 		{						\
> 			exitfn();				\
> 			rcu_barrier();				\
> 		}						\
> 	static inline exitcall_t __exittest(void)		\
> 	{ return exitfn; }					\
> 	void cleanup_module(void) __attribute__((alias("__exit_rcu_barrier")));

> Johannes are you fine with this?

Frankly, I don't think I really understand it, but if it works, why not?
I guess I could stick a printk into that function there to verify it ;-)

johannes

Re: compat-wireless: backporting kfree_rcu()?

[Luis R. Rodriguez]

T24gTW9uLCBOb3YgMTQsIDIwMTEgYXQgMTI6MjQgUE0sIEpvaGFubmVzIEJlcmcKPGpvaGFubmVz
QHNpcHNvbHV0aW9ucy5uZXQ+IHdyb3RlOgo+IEhpLAo+Cj4+ID4gI2RlZmluZSBrZnJlZV9yY3Uo
ZGF0YSwgcmN1aGVhZCkgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqBkbyB7IMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIMKgXAo+PiA+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIHZvaWQgX19rZnJl
ZV9yY3VfZm4oc3RydWN0IHJjdV9oZWFkICpyY3VfaGVhZCkgwqBcCj4+ID4gwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgeyDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBcCj4+ID4gwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgdm9pZCAqX19fcHRyOyDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBcCj4+ID4gwqAgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgX19fcHRyID0gY29udGFpbmVyX29mKHJjdV9oZWFkLCB0eXBlb2Yo
KihkYXRhKSksIHJjdWhlYWQpO1wKPj4gPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCBrZnJlZShfX19wdHIpOyDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCDCoCDCoCDCoCDCoFwKPj4gPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCB9IMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIFwKPj4gPiDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCBjYWxsX3JjdSgmKGRh
dGEpLT5yY3VoZWFkLCBfX2tmcmVlX3JjdV9mbik7IMKgIMKgIMKgIMKgIMKgIMKgIFwKPj4gPiDC
oCDCoCDCoCDCoCB9IHdoaWxlICgwKQo+PiA+Cj4+ID4KPj4gPiBUaGlzIHdvcmtzLCB0aGFua3Mg
dG8gZ2NjIHN1cHBvcnRpbmcgbmVzdGVkIGZ1bmN0aW9ucywgYnV0IGhhcyBvbmUgbWFqb3IKPj4g
PiBpc3N1ZTogYW55IG1vZHVsZSB1c2luZyBjYWxsX3JjdSgpIG11c3QgaGF2ZSBhbiByY3VfYmFy
cmllcigpIGluIGl0cwo+PiA+IG1vZHVsZV9leGl0KCkgYmVjYXVzZSBfX2tmcmVlX3JjdV9mbigp
IG1pZ2h0IGJlIGNhbGxlZCBhZnRlciB0aGUgbW9kdWxlCj4+ID4gaXMgdW5sb2FkZWQuIEZvciBr
ZnJlZV9yY3UoKSB0aGlzIGlzbid0IG5lZWRlZCBzaW5jZSB0aGUgZnVuY3Rpb24gY2FsbGVkCj4+
ID4gbGl2ZXMgaW4gdGhlIGJhc2Uga2VybmVsLgo+Pgo+PiBUaGlzIGxvb2tzIG5pY2UgdG8gbWUu
Cj4KPiA6LSkKPgo+PiA+IEkgcGxheWVkIGFyb3VuZCB3aXRoIGluamVjdGluZyBhIGNhbGwgdG8g
cmN1X2JhcnJpZXIoKSBpbnRvIG1vZHVsZSBleGl0Cj4+ID4gYnkgbW9kaWZ5aW5nIG1vZHVsZV9l
eGl0KCkgYnV0IEkgY291bGRuJ3QgbWFrZSB0aGUgQ1BQIGRvIHRoYXQuIEFueW9uZQo+PiA+IGVs
c2UgaGF2ZSBhbnkgaWRlYXM/Cj4+Cj4+IEkgcGxheWVkIGFyb3VuZCB3aXRoIG1vZHVsZV9leGl0
IGFuZCBtb2RpZmllZCB0aGUgZGVmaW5lIGludG8gdGhlCj4+IGZvbGxvd2luZy4gTm93IHJjdV9i
YXJyaWVyKCkgZ2V0cyBjYWxsZWQgb24gZXZlcnkgbW9kdWxlX2V4aXQuIEFzCj4+IG1vZHVsZXMg
YXJlIG5vdCB1bmxvYWRlZCBzbyBvZnRlbiBpdCBzaG91bGQgbm90IGh1cmQgdGhhdCB0aGlzIGlz
IGFsc28KPj4gZG9uZSB3aGVuIHVubG9hZGluZyBhIG1vZHVsZSBub3QgdXNpbmcga2ZyZWVfcmN1
KCkuCj4KPiBPay4KPgo+PiAjdW5kZWYgbW9kdWxlX2V4aXQKPj4gI2RlZmluZSBtb2R1bGVfZXhp
dChleGl0Zm4pIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIFwKPj4gwqAgwqAgwqAgdm9pZCBfX2V4aXQgX19leGl0X3JjdV9iYXJyaWVyKHZvaWQpIMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgXAo+PiDCoCDCoCDCoCDCoCDCoCDCoCDCoCB7IMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIFwKPj4gwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgZXhpdGZuKCk7
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIFwKPj4gwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgcmN1X2JhcnJpZXIoKTsgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqBcCj4+IMKgIMKgIMKgIMKgIMKgIMKgIMKgIH0gwqAgwqAg
wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgXAo+PiDCoCDCoCDCoCBzdGF0aWMgaW5saW5lIGV4aXRjYWxsX3QgX19leGl0dGVzdCh2
b2lkKSDCoCDCoCDCoCDCoCDCoCDCoCDCoCBcCj4+IMKgIMKgIMKgIHsgcmV0dXJuIGV4aXRmbjsg
fSDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oFwKPj4gwqAgwqAgwqAgdm9pZCBjbGVhbnVwX21vZHVsZSh2b2lkKSBfX2F0dHJpYnV0ZV9fKChh
bGlhcygiX19leGl0X3JjdV9iYXJyaWVyIikpKTsKPgo+PiBKb2hhbm5lcyBhcmUgeW91IGZpbmUg
d2l0aCB0aGlzPwo+Cj4gRnJhbmtseSwgSSBkb24ndCB0aGluayBJIHJlYWxseSB1bmRlcnN0YW5k
IGl0LCBidXQgaWYgaXQgd29ya3MsIHdoeSBub3Q/Cj4gSSBndWVzcyBJIGNvdWxkIHN0aWNrIGEg
cHJpbnRrIGludG8gdGhhdCBmdW5jdGlvbiB0aGVyZSB0byB2ZXJpZnkgaXQgOy0pCgpDYW4ndCBz
YXkgSSBnZXQgdGhpcyBlaXRoZXIsIGJ1dCBpZiBpdCB3b3JrIGFuZCBpZiBkb2N1bWVudGVkIHJl
YWxseQp3ZWxsIEknZCBnbGFkbHkgc3VjayBpdCBpbi4KCiAgTHVpcwo=