On Fri, Jul 02, 2010 at 01:07:44PM +0400, Dan Kruchinin wrote:
> >
> > The above is not save against cpu hotplug. You initialize ctx->cb_cpu
> > to -1 when the transformation is initialized. So you choose your
> > callback cpu with the first call to pcrypt_do_parallel() and then never
> > again. What if the coosen callback cpu goes offline?
> >
> > Also I don't understand why you changed the prototype of pcrypt_do_parallel
> > and added a 'count' to pcrypt_aead_ctx. The only change you have to do, is
> > to
> > check whether the choosen callback cpu is still in the cpu_active_mask
> > _and_
> > in your new padata callback cpumask and update the callback cpu accordingly
> > if not. Checking whether the callback cpu is in the callback cpumask needs
> > some special care in your other patch of course, because you can change
> > this
> > cpumask from userspace then.
> >
>
> Well, my point was to reduce the code and select callback CPU only once
> according to
> serial cpumask of given data instance. In case when I modify serial cpumask
> of given padata
> instance I have to do callback cpu calculation twice in worst case. The
> first time in pcrypt_aead_init_tfm
> and the second time in pcrypt_do_parallel if cb_cpu is not set in my serial
> cpumask.
> So I decided to do it only once in pcrypt_do_parallel.
>
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.
Steffen
On Fri, Jul 2, 2010 at 1:17 PM, Steffen Klassert
<[email protected]> wrote:
> On Fri, Jul 02, 2010 at 01:07:44PM +0400, Dan Kruchinin wrote:
>> >
>> > The above is not save against cpu hotplug. You initialize ctx->cb_cpu
>> > to -1 when the transformation is initialized. So you choose your
>> > callback cpu with the first call to pcrypt_do_parallel() and then never
>> > again. What if the coosen callback cpu goes offline?
>> >
>> > Also I don't understand why you changed the prototype of pcrypt_do_parallel
>> > and added a 'count' to pcrypt_aead_ctx. The only change you have to do, is
>> > to
>> > check whether the choosen callback cpu is still in the cpu_active_mask
>> > _and_
>> > in your new padata callback cpumask and update the callback cpu accordingly
>> > if not. Checking whether the callback cpu is in the callback cpumask needs
>> > some special care in your other patch of course, because you can change
>> > this
>> > cpumask from userspace then.
>> >
>>
>> Well, my point was to reduce the code and select callback CPU only once
>> according to
>> serial cpumask of given data instance. In case when I modify serial cpumask
>> of given padata
>> instance I have to do callback cpu calculation twice in worst case. The
>> first time in pcrypt_aead_init_tfm
>> and the second time in pcrypt_do_parallel if cb_cpu is not set in my serial
>> cpumask.
>> So I decided to do it only once in pcrypt_do_parallel.
>>
>
> 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.
>
> Steffen
>
>
--
W.B.R.
Dan Kruchinin
On Fri, Jul 2, 2010 at 1:32 PM, Dan Kruchinin <[email protected]> wrote:
> On Fri, Jul 2, 2010 at 1:17 PM, Steffen Klassert
> <[email protected]> wrote:
>> On Fri, Jul 02, 2010 at 01:07:44PM +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.
padata_do_parallel of course.
> 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.
>
>>
>> Steffen
>>
>>
>
>
>
> --
> W.B.R.
> Dan Kruchinin
>
--
W.B.R.
Dan Kruchinin
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.
Steffen
On Fri, Jul 2, 2010 at 3:11 PM, Steffen Klassert
<[email protected]> 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
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