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
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
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
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=
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
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