| Message ID | 20190716125704.l2jolyyd3bue6hhn@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | padata: Use RCU when fetching pd from do_serial | expand |
On Tue, Jul 16, 2019 at 08:57:04PM +0800, Herbert Xu wrote: > > How about using RCU? > > We still need to fix up the refcnt if it's supposed to limit the > overall number of outstanding requests. Hmm, it doesn't work because the refcnt is attached to the old pd. That shouldn't be a problem though as we could simply ignore the refcnt in padata_flush_queue. However, I think this leads to another bug in that pcrypt doesn't support dm-crypt properly. It never does the backlog stuff and therefore can't guarantee reliable processing which dm-crypt requires. Is it intentional to only allow pcrypt for IPsec? Cheers,
On Tue, Jul 16, 2019 at 08:57:04PM +0800, Herbert Xu wrote: > On Tue, Jul 16, 2019 at 01:14:10PM +0200, Steffen Klassert wrote: > > > > Maybe we can fix it if we call padata_free_pd() from > > padata_serial_worker() when it sent out the last object. > > How about using RCU? > > We still need to fix up the refcnt if it's supposed to limit the > overall number of outstanding requests. > > ---8<--- > The function padata_do_serial uses parallel_data without obeying > the RCU rules around its life-cycle. This means that a concurrent > padata_replace call can result in a crash. > > This patch fixes it by using RCU just as we do in padata_do_parallel. > > Fixes: 16295bec6398 ("padata: Generic parallelization/...") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > diff --git a/kernel/padata.c b/kernel/padata.c > index 2d2fddbb7a4c..fb5dd1210d2b 100644 > --- a/kernel/padata.c > +++ b/kernel/padata.c > @@ -128,7 +128,7 @@ int padata_do_parallel(struct padata_instance *pinst, > > err = 0; > atomic_inc(&pd->refcnt); > - padata->pd = pd; > + padata->inst = pinst; > padata->cb_cpu = cb_cpu; > > target_cpu = padata_cpu_hash(pd); > @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata) > struct parallel_data *pd; > int reorder_via_wq = 0; > > - pd = padata->pd; > + pd = rcu_dereference_bh(padata->inst->pd); > > cpu = get_cpu(); > That's weird for not having a matching assign and lacking comments to explain that.
On Tue, Jul 16, 2019 at 03:15:20PM +0200, Peter Zijlstra wrote: > > > @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata) > > struct parallel_data *pd; > > int reorder_via_wq = 0; > > > > - pd = padata->pd; > > + pd = rcu_dereference_bh(padata->inst->pd); > > > > cpu = get_cpu(); > > > > That's weird for not having a matching assign and lacking comments to > explain that. There is a matching rcu_assign_pointer. But we should add some RCU markers. Or perhaps you're misreading the level of indirections :) Cheers,
On Tue, Jul 16, 2019 at 09:18:07PM +0800, Herbert Xu wrote: > On Tue, Jul 16, 2019 at 03:15:20PM +0200, Peter Zijlstra wrote: > > > > > @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata) > > > struct parallel_data *pd; > > > int reorder_via_wq = 0; > > > > > > - pd = padata->pd; > > > + pd = rcu_dereference_bh(padata->inst->pd); > > > > > > cpu = get_cpu(); > > > > > > > That's weird for not having a matching assign and lacking comments to > > explain that. > > There is a matching rcu_assign_pointer. But we should add some > RCU markers. > > Or perhaps you're misreading the level of indirections :) Could well be, I'm not much familiar with this code; I'll look more careful.
On Tue, Jul 16, 2019 at 03:50:10PM +0200, Peter Zijlstra wrote: > > Could well be, I'm not much familiar with this code; I'll look more > careful. My patch is broken anyway. We can't just switch pd structures as the code relies on going to exactly the same CPU on the starting pd to ensure ordering. Cheers,
On Tue, Jul 16, 2019 at 09:09:28PM +0800, Herbert Xu wrote: > > However, I think this leads to another bug in that pcrypt doesn't > support dm-crypt properly. It never does the backlog stuff and > therefore can't guarantee reliable processing which dm-crypt requires. > > Is it intentional to only allow pcrypt for IPsec? I had a patch to support crypto backlog some years ago, but testing with dm-crypt did not show any performance improvement. So I decided to just skip that patch because it added code for no need.
On Wed, Jul 17, 2019 at 10:28:15AM +0200, Steffen Klassert wrote: > > I had a patch to support crypto backlog some years ago, > but testing with dm-crypt did not show any performance > improvement. So I decided to just skip that patch because > it added code for no need. Well pcrypt is part of the API so it needs to obey the rules. So even if it wouldn't make sense to use dm-crypt with pcrypt it still needs to do the right thing. Thanks,`
On Wed, Jul 17, 2019 at 04:47:40PM +0800, Herbert Xu wrote: > On Wed, Jul 17, 2019 at 10:28:15AM +0200, Steffen Klassert wrote: > > > > I had a patch to support crypto backlog some years ago, > > but testing with dm-crypt did not show any performance > > improvement. So I decided to just skip that patch because > > it added code for no need. > > Well pcrypt is part of the API so it needs to obey the rules. > So even if it wouldn't make sense to use dm-crypt with pcrypt > it still needs to do the right thing. Ok, makes sense. The old patch still exists: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/commit/?h=net-next-pcrypt&id=5909a88ccef6f4c78fe9969160155a8f0ce8fee7 Let me see if I can respin it...
diff --git a/include/linux/padata.h b/include/linux/padata.h index 5d13d25da2c8..952f6514dd72 100644 --- a/include/linux/padata.h +++ b/include/linux/padata.h @@ -35,7 +35,7 @@ * struct padata_priv - Embedded to the users data structure. * * @list: List entry, to attach to the padata lists. - * @pd: Pointer to the internal control structure. + * @inst: Pointer to the overall control structure. * @cb_cpu: Callback cpu for serializatioon. * @cpu: Cpu for parallelization. * @seq_nr: Sequence number of the parallelized data object. @@ -45,7 +45,7 @@ */ struct padata_priv { struct list_head list; - struct parallel_data *pd; + struct padata_instance *inst; int cb_cpu; int cpu; int info; diff --git a/kernel/padata.c b/kernel/padata.c index 2d2fddbb7a4c..fb5dd1210d2b 100644 --- a/kernel/padata.c +++ b/kernel/padata.c @@ -128,7 +128,7 @@ int padata_do_parallel(struct padata_instance *pinst, err = 0; atomic_inc(&pd->refcnt); - padata->pd = pd; + padata->inst = pinst; padata->cb_cpu = cb_cpu; target_cpu = padata_cpu_hash(pd); @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata) struct parallel_data *pd; int reorder_via_wq = 0; - pd = padata->pd; + pd = rcu_dereference_bh(padata->inst->pd); cpu = get_cpu();