You removed everyone in the Cc, please don't do this unless you have
good reason for that. I've added the Cc'ed people again, perhaps
somebody has an opinion on this too.
On Tue, Jul 06, 2010 at 07:36:12AM +0200, Steffen Klassert wrote:
> On Mon, Jul 05, 2010 at 06:09:17PM +0400, Dan Kruchinin wrote:
> >
> > I tried to implement RCU protection on cpumask, but it appears a bit
> > ugly because we can not safely assign cpumask_var_t(that is allocated
> > via alloc_cpumask_var) to struct cpumask* via rcu_assign_pointer. The
> > root of problem lies in cpumask_var_t definition. Depending on
> > CONFIG_CPUMASK_OFFSTACK macro it may be a local variable on the stack
> > or a pointer to struct cpumask:
> > #ifdef CONFIG_CPUMASK_OFFSTACK
> > typedef struct cpumask *cpumask_var_t
> > ...
> > #else
> > typedef struct cpumask cpumask_var_t[1];
> > ...
> > #endif
>
> Hm, yes dealing with cpumasks is a bit special these days.
>
> >
> > In this case rcu_assign_pointer may be safely used only if we deal
> > with a pointer to cpumask_var_t that must be allocated via kmalloc
> > before using alloc_cpumask_var and rcu_assign_pointer. I think it's -
> > as I said earlier - a bit ugly and hard to read.
> > My be it's better to use rw spinlock instead? In our situation it
> > doesn't significantly differ from RCU. Also it'll make code more clear
> > and easy to read.
> >
>
> I think we can use RCU anyway. For instance we could use a structure
>
> struct pcrypt_cpumask {
> cpumask_var_t pmask;
> cpumask_var_t smask;
> };
>
> and add a pointer to a structure of that type to the instance context.
> Then we could use this pointer for RCU and replace the whole structure
> if a cpumask changes.
>
>
On Tue, Jul 6, 2010 at 9:44 AM, Steffen Klassert
<[email protected]> wrote:
> You removed everyone in the Cc, please don't do this unless you have
> good reason for that. I've added the Cc'ed people again, perhaps
> somebody has an opinion on this too.
Oh, I'm sorry I didn't notice.
>
> On Tue, Jul 06, 2010 at 07:36:12AM +0200, Steffen Klassert wrote:
>> On Mon, Jul 05, 2010 at 06:09:17PM +0400, Dan Kruchinin wrote:
>> >
>> > I tried to implement RCU protection on cpumask, but it appears a bit
>> > ugly because we can not safely assign cpumask_var_t(that is allocated
>> > via alloc_cpumask_var) to struct cpumask* via rcu_assign_pointer. The
>> > root of problem lies in cpumask_var_t definition. Depending on
>> > CONFIG_CPUMASK_OFFSTACK macro it may be a local variable on the stack
>> > or a pointer to struct cpumask:
>> > #ifdef CONFIG_CPUMASK_OFFSTACK
>> > typedef struct cpumask *cpumask_var_t
>> > ...
>> > #else
>> > typedef struct cpumask cpumask_var_t[1];
>> > ...
>> > #endif
>>
>> Hm, yes dealing with cpumasks is a bit special these days.
>>
>> >
>> > In this case rcu_assign_pointer may be safely used only if we deal
>> > with a pointer to cpumask_var_t that must be allocated via kmalloc
>> > before using alloc_cpumask_var and rcu_assign_pointer. I think it's -
>> > as I said earlier - a bit ugly and hard to read.
>> > My be it's better to use rw spinlock instead? In our situation it
>> > doesn't significantly differ from RCU. Also it'll make code more clear
>> > and easy to read.
>> >
>>
>> I think we can use RCU anyway. For instance we could use a structure
>>
>> struct pcrypt_cpumask {
>> ? ? ? cpumask_var_t ? ? ? ? ? pmask;
>> ? ? ? cpumask_var_t ? ? ? ? ? smask;
>> };
>>
>> and add a pointer to a structure of that type to the instance context.
>> Then we could use this pointer for RCU and replace the whole structure
>> if a cpumask changes.
But is pcrypt interested pmask? If it isn't, pmask field will be unused.
>>
>>
>
--
W.B.R.
Dan Kruchinin
On Tue, Jul 06, 2010 at 11:40:11AM +0400, Dan Kruchinin wrote:
> >> >
> >>
> >> I think we can use RCU anyway. For instance we could use a structure
> >>
> >> struct pcrypt_cpumask {
> >> ? ? ? cpumask_var_t ? ? ? ? ? pmask;
> >> ? ? ? cpumask_var_t ? ? ? ? ? smask;
> >> };
> >>
> >> and add a pointer to a structure of that type to the instance context.
> >> Then we could use this pointer for RCU and replace the whole structure
> >> if a cpumask changes.
>
> But is pcrypt interested pmask? If it isn't, pmask field will be unused.
>
It's probaply not, in this case the struct could look like
struct pcrypt_cpumask {
cpumask_var_t ? ? ? ? ? smask;
};
On Tue, Jul 6, 2010 at 11:46 AM, Steffen Klassert
<[email protected]> wrote:
> On Tue, Jul 06, 2010 at 11:40:11AM +0400, Dan Kruchinin wrote:
>> >> >
>> >>
>> >> I think we can use RCU anyway. For instance we could use a structure
>> >>
>> >> struct pcrypt_cpumask {
>> >> ? ? ? cpumask_var_t ? ? ? ? ? pmask;
>> >> ? ? ? cpumask_var_t ? ? ? ? ? smask;
>> >> };
>> >>
>> >> and add a pointer to a structure of that type to the instance context.
>> >> Then we could use this pointer for RCU and replace the whole structure
>> >> if a cpumask changes.
>>
>> But is pcrypt interested pmask? If it isn't, pmask field will be unused.
>>
>
> It's probaply not, in this case the struct could look like
>
> struct pcrypt_cpumask {
> ? ? ? ?cpumask_var_t ? ? ? ? ? smask;
> };
>
Would't it be the same as with a pointer to cpumask_var_t? I mean:
struct pcrypt {
...
struct pcrypt_cpumask *mask;
...
} pencrypt;
To assign a pointer via RCU:
int cpumask_change_nitify(...) {
...
struct pcrypt_cpumask *new_mask = kmalloc(sizeof(*mask), GFP);
struct pcrypt_cpumask *old_mask = pencrypt.mask;
if (!new_mask)
error();
if (!alloc_cpumask_var(&new_mask->smask, GFP_KERNEL))
error();
get_serial_cpumask_from_padata(new_mask->mask);
rcu_assign_pointer(pencrypt.mask, new_mask);
synchronize_rcu_bh();
free_cpumask_var(old_mask->smask);
kfree(old_mask);
...
}
It's a bit hard to read this code because at the first sight it
appears unclear and odd why we allocate the structure with only one
member.
--
W.B.R.
Dan Kruchinin
On Tue, Jul 06, 2010 at 12:31:21PM +0400, Dan Kruchinin wrote:
>
> Would't it be the same as with a pointer to cpumask_var_t? I mean:
Using a pointer to cpumask_var_t is a bit problematic because
you don't know a priori about the type of cpumask_var_t.
The type depends whether the cpumasks are on/off stack.
So the easiest thing is to embed it to a struct, then you don't
need to care about the type. If you allocate a struct of type
pcrypt_cpumask you get what you want to have.
> struct pcrypt {
> ...
> struct pcrypt_cpumask *mask;
> ...
> } pencrypt;
>
> To assign a pointer via RCU:
>
> int cpumask_change_nitify(...) {
> ...
> struct pcrypt_cpumask *new_mask = kmalloc(sizeof(*mask), GFP);
> struct pcrypt_cpumask *old_mask = pencrypt.mask;
>
> if (!new_mask)
> error();
> if (!alloc_cpumask_var(&new_mask->smask, GFP_KERNEL))
> error();
>
> get_serial_cpumask_from_padata(new_mask->mask);
> rcu_assign_pointer(pencrypt.mask, new_mask);
> synchronize_rcu_bh();
>
> free_cpumask_var(old_mask->smask);
> kfree(old_mask);
> ...
> }
>
> It's a bit hard to read this code because at the first sight it
> appears unclear and odd why we allocate the structure with only one
> member.
>
We can easily add a code comment if this appears to be unclear :)
On Tue, Jul 6, 2010 at 5:28 PM, Steffen Klassert
<[email protected]> wrote:
> On Tue, Jul 06, 2010 at 12:31:21PM +0400, Dan Kruchinin wrote:
>>
>> Would't it be the same as with a pointer to cpumask_var_t? I mean:
>
> Using a pointer to cpumask_var_t is a bit problematic because
> you don't know a priori about the type of cpumask_var_t.
> The type depends whether the cpumasks are on/off stack.
> So the easiest thing is to embed it to a struct, then you don't
> need to care about the type. If you allocate a struct of type
> pcrypt_cpumask you get what you want to have.
Oh, I meant pointer to pointer of course. Anyway you're probably right.
I modified my patches according to the results of our discussion. So
I'm waiting for your fixes.
>
>> struct pcrypt {
>> ? ...
>> ? struct pcrypt_cpumask *mask;
>> ? ...
>> } pencrypt;
>>
>> To assign a pointer via RCU:
>>
>> int cpumask_change_nitify(...) {
>> ? ?...
>> ? struct pcrypt_cpumask *new_mask = kmalloc(sizeof(*mask), GFP);
>> ? struct pcrypt_cpumask *old_mask = pencrypt.mask;
>>
>> ? if (!new_mask)
>> ? ? ?error();
>> ? if (!alloc_cpumask_var(&new_mask->smask, GFP_KERNEL))
>> ? ? ?error();
>>
>> ? get_serial_cpumask_from_padata(new_mask->mask);
>> ? rcu_assign_pointer(pencrypt.mask, new_mask);
>> ? synchronize_rcu_bh();
>>
>> ? free_cpumask_var(old_mask->smask);
>> ? kfree(old_mask);
>> ? ...
>> }
>>
>> It's a bit hard to read this code because at the first sight it
>> appears unclear and odd why we allocate the structure with only one
>> member.
>>
>
> We can easily add a code comment if this appears to be unclear :)
>
--
W.B.R.
Dan Kruchinin
On Tue, Jul 06, 2010 at 08:30:40PM +0400, Dan Kruchinin wrote:
>
> I modified my patches according to the results of our discussion. So
> I'm waiting for your fixes.
>
I'll send out a patchset later. The first three patches of the patchset
address the empty padata cpumask handling. I did some rough tests and it
appeared to work. However, I still have no fixed testcase for this. So see
whether this works with your patches and whether it meets your requirements.