by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH 23/23] locktorture: Check the correct variable for allocation failure

On Tue, Oct 10, 2023 at 05:07:00PM +0300, Dan Carpenter wrote:
> On Tue, Oct 10, 2023 at 06:55:40AM -0700, Paul E. McKenney wrote:
> > On Tue, Oct 10, 2023 at 01:59:21PM +0200, Frederic Weisbecker wrote:
> > > From: Dan Carpenter <[email protected]>
> > >
> > > There is a typo so this checks the wrong variable. "chains" plural vs
> > > "chain" singular. We already know that "chains" is non-zero.
> > >
> > > Fixes: 7f993623e9eb ("locktorture: Add call_rcu_chains module parameter")
> > > Signed-off-by: Dan Carpenter <[email protected]>
> > > Signed-off-by: Frederic Weisbecker <[email protected]>
> >
> > Reviewed-by: Paul E. McKenney <[email protected]>
> >
> > A name change to increase the Hamming distance would of course also be
> > good, though less urgent. ;-)
>
> "Hamming distance" is such a great phrase. I'm going to use that every
> time I complain about confusingly similar variable names going forward.

Glad you like it!

But the horrible thing is that I first heard that phrase back in
the 1970s, and I am the guilty party who created these particular
too-similar variable names. (Why has the phrase fallen out of favor?
No idea, really, but one guess has to do with the fact that current
error-correcting codes must deal with different probabilities of different
bits flipping in different directions, so you would instead needs a
weirdly weighted variant of Hamming distance to accomplish anything with
modern error-correcting codes.)

But how about something like the following?

Thanx, paul

------------------------------------------------------------------------

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 991496afc0d9..48fd4562a754 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -126,7 +126,7 @@ struct call_rcu_chain {
struct rcu_head crc_rh;
bool crc_stop;
};
-struct call_rcu_chain *call_rcu_chain;
+struct call_rcu_chain *call_rcu_chain_list;

/* Forward reference. */
static void lock_torture_cleanup(void);
@@ -1106,12 +1106,12 @@ static int call_rcu_chain_init(void)

if (call_rcu_chains <= 0)
return 0;
- call_rcu_chain = kcalloc(call_rcu_chains, sizeof(*call_rcu_chain), GFP_KERNEL);
- if (!call_rcu_chain)
+ call_rcu_chain_list = kcalloc(call_rcu_chains, sizeof(*call_rcu_chain_list), GFP_KERNEL);
+ if (!call_rcu_chain_list)
return -ENOMEM;
for (i = 0; i < call_rcu_chains; i++) {
- call_rcu_chain[i].crc_stop = false;
- call_rcu(&call_rcu_chain[i].crc_rh, call_rcu_chain_cb);
+ call_rcu_chain_list[i].crc_stop = false;
+ call_rcu(&call_rcu_chain_list[i].crc_rh, call_rcu_chain_cb);
}
return 0;
}
@@ -1121,13 +1121,13 @@ static void call_rcu_chain_cleanup(void)
{
int i;

- if (!call_rcu_chain)
+ if (!call_rcu_chain_list)
return;
for (i = 0; i < call_rcu_chains; i++)
- smp_store_release(&call_rcu_chain[i].crc_stop, true);
+ smp_store_release(&call_rcu_chain_list[i].crc_stop, true);
rcu_barrier();
- kfree(call_rcu_chain);
- call_rcu_chain = NULL;
+ kfree(call_rcu_chain_list);
+ call_rcu_chain_list = NULL;
}

static struct notifier_block lock_torture_stall_block;

2023-10-11 06:46:45

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH 23/23] locktorture: Check the correct variable for allocation failure

On Tue, Oct 10, 2023 at 08:53:36AM -0700, Paul E. McKenney wrote:
> On Tue, Oct 10, 2023 at 05:07:00PM +0300, Dan Carpenter wrote:
> > On Tue, Oct 10, 2023 at 06:55:40AM -0700, Paul E. McKenney wrote:
> > > On Tue, Oct 10, 2023 at 01:59:21PM +0200, Frederic Weisbecker wrote:
> > > > From: Dan Carpenter <[email protected]>
> > > >
> > > > There is a typo so this checks the wrong variable. "chains" plural vs
> > > > "chain" singular. We already know that "chains" is non-zero.
> > > >
> > > > Fixes: 7f993623e9eb ("locktorture: Add call_rcu_chains module parameter")
> > > > Signed-off-by: Dan Carpenter <[email protected]>
> > > > Signed-off-by: Frederic Weisbecker <[email protected]>
> > >
> > > Reviewed-by: Paul E. McKenney <[email protected]>
> > >
> > > A name change to increase the Hamming distance would of course also be
> > > good, though less urgent. ;-)
> >
> > "Hamming distance" is such a great phrase. I'm going to use that every
> > time I complain about confusingly similar variable names going forward.
>
> Glad you like it!
>
> But the horrible thing is that I first heard that phrase back in
> the 1970s, and I am the guilty party who created these particular
> too-similar variable names. (Why has the phrase fallen out of favor?
> No idea, really, but one guess has to do with the fact that current
> error-correcting codes must deal with different probabilities of different
> bits flipping in different directions, so you would instead needs a
> weirdly weighted variant of Hamming distance to accomplish anything with
> modern error-correcting codes.)
>
> But how about something like the following?
>

Looks good!

regards,
dan carpenter