Prev: [PATCH 4/4] workqueue: fix worker management invocation without pending works
Next: pcrypt: sysfs interface
From: Dan Kruchinin on 2 Jul 2010 09:10
On Fri, Jul 2, 2010 at 3:11 PM, Steffen Klassert
<steffen.klassert(a)secunet.com> wrote:
> On Fri, Jul 02, 2010 at 01:32:29PM +0400, Dan Kruchinin wrote:
>> >
>> > But the active cpumask, and now also your serial cpumask might change.
>> > We need to catch this changes somehow, that's why I checked the active
>> > cpumask against the callback cpu.
>>
>> You're right, now I get it. Hence the right solution is to check if
>> callback CPU is set in serial cpumask every time we do
>> padata_do_serial and if it's not, recalculate its value.
>> The only thing that embarrasses me in this scheme is the fact that we
>> have to allocate cpumask_var_t in pcrypt_do_parallel every time we
>> call it then copy serial cpumask into allocated one and then check the
>> cb_cpu.
>> I think it would be better if we somehow could avoid dynamic cpumask
>> allocation. I see the following solutions:
>>
>> 1) Do the check and cb_cpu value recalculation in padata_do_parallel.
>> It may check if cb_cpu is in serial_cpumask and recalculate its value
>> if it isn't. The drawback of this scheme is that padata_do_parallel
>> now doesn't guaranty it will forward serialization job to the same
>> callback CPU we passed to it. If passed CPU is not in serial cpumask
>> it will forward serialization to another CPU and we won't know its
>> number. The only thing we'll know is that this CPU is in the
>> serial_cpumask.
>> 2) Create new structure describing pcrypt instance in pcrypt.c which
>> will include waitqueue, padata instance and preallocated cpumask which
>> will be used for getting padata instance serial cpumsak. It'll help to
>> avoid dynamic cpumask allocation but it looks a bit awkward.
>>
>
> I think the cleanest way to do it, is to maintain notifier chains
> for parallel/serial cpumask changes in padata. Users can register to
> these notifier chains if they are interestet in these events.
> pcrypt is probaply just in changes of the serial cpumsk interested,
> so you could alloc and initialize such a cpumask in pcrypt_aead_init_tfm
> and add a pointer to it to pcrypt_aead_ctx.
> Then you could update the cpumask with the notifier callback function.
> cpumask changes are rare and slow anyway, so copying the cpumask there does
> not matter that much. Since cpumask changes are rare, you can protect
> pcrypt_do_parallel with RCU against cpumask changes.

Sounds good. But if I understand linux crypto framework right, it
calls init_tfm every time it creates new security association. Ideally
pcrypt should have only two cpumasks one for pencrypt instance and
another for pdecrypt.
If we'll initialize these cpumasks in pcrypt_alloc_tfm they'll be
initialized every time new SA appears.
>
> Steffen
>



--
W.B.R.
Dan Kruchinin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steffen Klassert on 5 Jul 2010 07:00
On Fri, Jul 02, 2010 at 05:01:28PM +0400, Dan Kruchinin wrote:
>
> Sounds good. But if I understand linux crypto framework right, it
> calls init_tfm every time it creates new security association. Ideally
> pcrypt should have only two cpumasks one for pencrypt instance and
> another for pdecrypt.
> If we'll initialize these cpumasks in pcrypt_alloc_tfm they'll be
> initialized every time new SA appears.

Yes, you are absolutely right. Placing this to instance context
is the right way to do it.

Thanks,

Steffen


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steffen Klassert on 6 Jul 2010 01:50
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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dan Kruchinin on 6 Jul 2010 03:50
On Tue, Jul 6, 2010 at 9:44 AM, Steffen Klassert
<steffen.klassert(a)secunet.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steffen Klassert on 6 Jul 2010 03:50
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;
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH 4/4] workqueue: fix worker management invocation without pending works
Next: pcrypt: sysfs interface