From: Qiuxu Zhuo <[email protected]>
This code-movement-only commit moves the rcu_scale_cleanup() and
rcu_scale_shutdown() functions to follow kfree_scale_cleanup().
This is code movement is in preparation for a bug-fix patch that invokes
kfree_scale_cleanup() from rcu_scale_cleanup().
Signed-off-by: Qiuxu Zhuo <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
kernel/rcu/rcuscale.c | 194 +++++++++++++++++++++---------------------
1 file changed, 97 insertions(+), 97 deletions(-)
diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
index e82ec9f9a5d8..7e8965b0827a 100644
--- a/kernel/rcu/rcuscale.c
+++ b/kernel/rcu/rcuscale.c
@@ -522,89 +522,6 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *cur_ops, const char *tag)
scale_type, tag, nrealreaders, nrealwriters, verbose, shutdown);
}
-static void
-rcu_scale_cleanup(void)
-{
- int i;
- int j;
- int ngps = 0;
- u64 *wdp;
- u64 *wdpp;
-
- /*
- * Would like warning at start, but everything is expedited
- * during the mid-boot phase, so have to wait till the end.
- */
- if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
- SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
- if (rcu_gp_is_normal() && gp_exp)
- SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
- if (gp_exp && gp_async)
- SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
-
- if (torture_cleanup_begin())
- return;
- if (!cur_ops) {
- torture_cleanup_end();
- return;
- }
-
- if (reader_tasks) {
- for (i = 0; i < nrealreaders; i++)
- torture_stop_kthread(rcu_scale_reader,
- reader_tasks[i]);
- kfree(reader_tasks);
- }
-
- if (writer_tasks) {
- for (i = 0; i < nrealwriters; i++) {
- torture_stop_kthread(rcu_scale_writer,
- writer_tasks[i]);
- if (!writer_n_durations)
- continue;
- j = writer_n_durations[i];
- pr_alert("%s%s writer %d gps: %d\n",
- scale_type, SCALE_FLAG, i, j);
- ngps += j;
- }
- pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
- scale_type, SCALE_FLAG,
- t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
- t_rcu_scale_writer_finished -
- t_rcu_scale_writer_started,
- ngps,
- rcuscale_seq_diff(b_rcu_gp_test_finished,
- b_rcu_gp_test_started));
- for (i = 0; i < nrealwriters; i++) {
- if (!writer_durations)
- break;
- if (!writer_n_durations)
- continue;
- wdpp = writer_durations[i];
- if (!wdpp)
- continue;
- for (j = 0; j < writer_n_durations[i]; j++) {
- wdp = &wdpp[j];
- pr_alert("%s%s %4d writer-duration: %5d %llu\n",
- scale_type, SCALE_FLAG,
- i, j, *wdp);
- if (j % 100 == 0)
- schedule_timeout_uninterruptible(1);
- }
- kfree(writer_durations[i]);
- }
- kfree(writer_tasks);
- kfree(writer_durations);
- kfree(writer_n_durations);
- }
-
- /* Do torture-type-specific cleanup operations. */
- if (cur_ops->cleanup != NULL)
- cur_ops->cleanup();
-
- torture_cleanup_end();
-}
-
/*
* Return the number if non-negative. If -1, the number of CPUs.
* If less than -1, that much less than the number of CPUs, but
@@ -624,20 +541,6 @@ static int compute_real(int n)
return nr;
}
-/*
- * RCU scalability shutdown kthread. Just waits to be awakened, then shuts
- * down system.
- */
-static int
-rcu_scale_shutdown(void *arg)
-{
- wait_event_idle(shutdown_wq, atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
- smp_mb(); /* Wake before output. */
- rcu_scale_cleanup();
- kernel_power_off();
- return -EINVAL;
-}
-
/*
* kfree_rcu() scalability tests: Start a kfree_rcu() loop on all CPUs for number
* of iterations and measure total time and number of GP for all iterations to complete.
@@ -874,6 +777,103 @@ kfree_scale_init(void)
return firsterr;
}
+static void
+rcu_scale_cleanup(void)
+{
+ int i;
+ int j;
+ int ngps = 0;
+ u64 *wdp;
+ u64 *wdpp;
+
+ /*
+ * Would like warning at start, but everything is expedited
+ * during the mid-boot phase, so have to wait till the end.
+ */
+ if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
+ SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
+ if (rcu_gp_is_normal() && gp_exp)
+ SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
+ if (gp_exp && gp_async)
+ SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
+
+ if (torture_cleanup_begin())
+ return;
+ if (!cur_ops) {
+ torture_cleanup_end();
+ return;
+ }
+
+ if (reader_tasks) {
+ for (i = 0; i < nrealreaders; i++)
+ torture_stop_kthread(rcu_scale_reader,
+ reader_tasks[i]);
+ kfree(reader_tasks);
+ }
+
+ if (writer_tasks) {
+ for (i = 0; i < nrealwriters; i++) {
+ torture_stop_kthread(rcu_scale_writer,
+ writer_tasks[i]);
+ if (!writer_n_durations)
+ continue;
+ j = writer_n_durations[i];
+ pr_alert("%s%s writer %d gps: %d\n",
+ scale_type, SCALE_FLAG, i, j);
+ ngps += j;
+ }
+ pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
+ scale_type, SCALE_FLAG,
+ t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
+ t_rcu_scale_writer_finished -
+ t_rcu_scale_writer_started,
+ ngps,
+ rcuscale_seq_diff(b_rcu_gp_test_finished,
+ b_rcu_gp_test_started));
+ for (i = 0; i < nrealwriters; i++) {
+ if (!writer_durations)
+ break;
+ if (!writer_n_durations)
+ continue;
+ wdpp = writer_durations[i];
+ if (!wdpp)
+ continue;
+ for (j = 0; j < writer_n_durations[i]; j++) {
+ wdp = &wdpp[j];
+ pr_alert("%s%s %4d writer-duration: %5d %llu\n",
+ scale_type, SCALE_FLAG,
+ i, j, *wdp);
+ if (j % 100 == 0)
+ schedule_timeout_uninterruptible(1);
+ }
+ kfree(writer_durations[i]);
+ }
+ kfree(writer_tasks);
+ kfree(writer_durations);
+ kfree(writer_n_durations);
+ }
+
+ /* Do torture-type-specific cleanup operations. */
+ if (cur_ops->cleanup != NULL)
+ cur_ops->cleanup();
+
+ torture_cleanup_end();
+}
+
+/*
+ * RCU scalability shutdown kthread. Just waits to be awakened, then shuts
+ * down system.
+ */
+static int
+rcu_scale_shutdown(void *arg)
+{
+ wait_event_idle(shutdown_wq, atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
+ smp_mb(); /* Wake before output. */
+ rcu_scale_cleanup();
+ kernel_power_off();
+ return -EINVAL;
+}
+
static int __init
rcu_scale_init(void)
{
--
2.40.1
On Wed, May 10, 2023 at 10:12 AM Paul E. McKenney <[email protected]> wrote:
>
> From: Qiuxu Zhuo <[email protected]>
>
> This code-movement-only commit moves the rcu_scale_cleanup() and
> rcu_scale_shutdown() functions to follow kfree_scale_cleanup().
> This is code movement is in preparation for a bug-fix patch that invokes
> kfree_scale_cleanup() from rcu_scale_cleanup().
>
> Signed-off-by: Qiuxu Zhuo <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> ---
> kernel/rcu/rcuscale.c | 194 +++++++++++++++++++++---------------------
> 1 file changed, 97 insertions(+), 97 deletions(-)
I wish diff was better at showing what really changed. The meld tool
can help but its gui...
Should I run meld later (I'm out at a conference so no access to
meld-capable machines) or are we sufficiently confident that the lines
were moved as-is ? :)
- Joel
>
> diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> index e82ec9f9a5d8..7e8965b0827a 100644
> --- a/kernel/rcu/rcuscale.c
> +++ b/kernel/rcu/rcuscale.c
> @@ -522,89 +522,6 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *cur_ops, const char *tag)
> scale_type, tag, nrealreaders, nrealwriters, verbose, shutdown);
> }
>
> -static void
> -rcu_scale_cleanup(void)
> -{
> - int i;
> - int j;
> - int ngps = 0;
> - u64 *wdp;
> - u64 *wdpp;
> -
> - /*
> - * Would like warning at start, but everything is expedited
> - * during the mid-boot phase, so have to wait till the end.
> - */
> - if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> - SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> - if (rcu_gp_is_normal() && gp_exp)
> - SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> - if (gp_exp && gp_async)
> - SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
> -
> - if (torture_cleanup_begin())
> - return;
> - if (!cur_ops) {
> - torture_cleanup_end();
> - return;
> - }
> -
> - if (reader_tasks) {
> - for (i = 0; i < nrealreaders; i++)
> - torture_stop_kthread(rcu_scale_reader,
> - reader_tasks[i]);
> - kfree(reader_tasks);
> - }
> -
> - if (writer_tasks) {
> - for (i = 0; i < nrealwriters; i++) {
> - torture_stop_kthread(rcu_scale_writer,
> - writer_tasks[i]);
> - if (!writer_n_durations)
> - continue;
> - j = writer_n_durations[i];
> - pr_alert("%s%s writer %d gps: %d\n",
> - scale_type, SCALE_FLAG, i, j);
> - ngps += j;
> - }
> - pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> - scale_type, SCALE_FLAG,
> - t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> - t_rcu_scale_writer_finished -
> - t_rcu_scale_writer_started,
> - ngps,
> - rcuscale_seq_diff(b_rcu_gp_test_finished,
> - b_rcu_gp_test_started));
> - for (i = 0; i < nrealwriters; i++) {
> - if (!writer_durations)
> - break;
> - if (!writer_n_durations)
> - continue;
> - wdpp = writer_durations[i];
> - if (!wdpp)
> - continue;
> - for (j = 0; j < writer_n_durations[i]; j++) {
> - wdp = &wdpp[j];
> - pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> - scale_type, SCALE_FLAG,
> - i, j, *wdp);
> - if (j % 100 == 0)
> - schedule_timeout_uninterruptible(1);
> - }
> - kfree(writer_durations[i]);
> - }
> - kfree(writer_tasks);
> - kfree(writer_durations);
> - kfree(writer_n_durations);
> - }
> -
> - /* Do torture-type-specific cleanup operations. */
> - if (cur_ops->cleanup != NULL)
> - cur_ops->cleanup();
> -
> - torture_cleanup_end();
> -}
> -
> /*
> * Return the number if non-negative. If -1, the number of CPUs.
> * If less than -1, that much less than the number of CPUs, but
> @@ -624,20 +541,6 @@ static int compute_real(int n)
> return nr;
> }
>
> -/*
> - * RCU scalability shutdown kthread. Just waits to be awakened, then shuts
> - * down system.
> - */
> -static int
> -rcu_scale_shutdown(void *arg)
> -{
> - wait_event_idle(shutdown_wq, atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> - smp_mb(); /* Wake before output. */
> - rcu_scale_cleanup();
> - kernel_power_off();
> - return -EINVAL;
> -}
> -
> /*
> * kfree_rcu() scalability tests: Start a kfree_rcu() loop on all CPUs for number
> * of iterations and measure total time and number of GP for all iterations to complete.
> @@ -874,6 +777,103 @@ kfree_scale_init(void)
> return firsterr;
> }
>
> +static void
> +rcu_scale_cleanup(void)
> +{
> + int i;
> + int j;
> + int ngps = 0;
> + u64 *wdp;
> + u64 *wdpp;
> +
> + /*
> + * Would like warning at start, but everything is expedited
> + * during the mid-boot phase, so have to wait till the end.
> + */
> + if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> + SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> + if (rcu_gp_is_normal() && gp_exp)
> + SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> + if (gp_exp && gp_async)
> + SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
> +
> + if (torture_cleanup_begin())
> + return;
> + if (!cur_ops) {
> + torture_cleanup_end();
> + return;
> + }
> +
> + if (reader_tasks) {
> + for (i = 0; i < nrealreaders; i++)
> + torture_stop_kthread(rcu_scale_reader,
> + reader_tasks[i]);
> + kfree(reader_tasks);
> + }
> +
> + if (writer_tasks) {
> + for (i = 0; i < nrealwriters; i++) {
> + torture_stop_kthread(rcu_scale_writer,
> + writer_tasks[i]);
> + if (!writer_n_durations)
> + continue;
> + j = writer_n_durations[i];
> + pr_alert("%s%s writer %d gps: %d\n",
> + scale_type, SCALE_FLAG, i, j);
> + ngps += j;
> + }
> + pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> + scale_type, SCALE_FLAG,
> + t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> + t_rcu_scale_writer_finished -
> + t_rcu_scale_writer_started,
> + ngps,
> + rcuscale_seq_diff(b_rcu_gp_test_finished,
> + b_rcu_gp_test_started));
> + for (i = 0; i < nrealwriters; i++) {
> + if (!writer_durations)
> + break;
> + if (!writer_n_durations)
> + continue;
> + wdpp = writer_durations[i];
> + if (!wdpp)
> + continue;
> + for (j = 0; j < writer_n_durations[i]; j++) {
> + wdp = &wdpp[j];
> + pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> + scale_type, SCALE_FLAG,
> + i, j, *wdp);
> + if (j % 100 == 0)
> + schedule_timeout_uninterruptible(1);
> + }
> + kfree(writer_durations[i]);
> + }
> + kfree(writer_tasks);
> + kfree(writer_durations);
> + kfree(writer_n_durations);
> + }
> +
> + /* Do torture-type-specific cleanup operations. */
> + if (cur_ops->cleanup != NULL)
> + cur_ops->cleanup();
> +
> + torture_cleanup_end();
> +}
> +
> +/*
> + * RCU scalability shutdown kthread. Just waits to be awakened, then shuts
> + * down system.
> + */
> +static int
> +rcu_scale_shutdown(void *arg)
> +{
> + wait_event_idle(shutdown_wq, atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> + smp_mb(); /* Wake before output. */
> + rcu_scale_cleanup();
> + kernel_power_off();
> + return -EINVAL;
> +}
> +
> static int __init
> rcu_scale_init(void)
> {
> --
> 2.40.1
>
> From: Joel Fernandes <[email protected]>
> Sent: Thursday, May 11, 2023 1:23 PM
> To: Paul E. McKenney <[email protected]>
> Cc: [email protected]; [email protected]; kernel-
> [email protected]; [email protected]; Zhuo, Qiuxu
> <[email protected]>
> Subject: Re: [PATCH rcu 3/6] rcu/rcuscale: Move rcu_scale_*() after
> kfree_scale_cleanup()
>
> On Wed, May 10, 2023 at 10:12 AM Paul E. McKenney <[email protected]>
> wrote:
> >
> > From: Qiuxu Zhuo <[email protected]>
> >
> > This code-movement-only commit moves the rcu_scale_cleanup() and
> > rcu_scale_shutdown() functions to follow kfree_scale_cleanup().
> > This is code movement is in preparation for a bug-fix patch that
> > invokes
> > kfree_scale_cleanup() from rcu_scale_cleanup().
> >
> > Signed-off-by: Qiuxu Zhuo <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > ---
> > kernel/rcu/rcuscale.c | 194
> > +++++++++++++++++++++---------------------
> > 1 file changed, 97 insertions(+), 97 deletions(-)
>
> I wish diff was better at showing what really changed. The meld tool can help
> but its gui...
>
> Should I run meld later (I'm out at a conference so no access to meld-capable
> machines) or are we sufficiently confident that the lines were moved as-is ? :)
>
Thank you, Joel for this concern. Good to know the meld diff GUI tool.
I just run the command below and confirmed that the lines were moved
as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
You may double check it ;-).
meld --diff ./rcuscale.c.before ./rcuscale.c.after
-Qiuxu
> - Joel
On Thu, May 11, 2023 at 07:01:59AM +0000, Zhuo, Qiuxu wrote:
> > From: Joel Fernandes <[email protected]>
> > Sent: Thursday, May 11, 2023 1:23 PM
> > To: Paul E. McKenney <[email protected]>
> > Cc: [email protected]; [email protected]; kernel-
> > [email protected]; [email protected]; Zhuo, Qiuxu
> > <[email protected]>
> > Subject: Re: [PATCH rcu 3/6] rcu/rcuscale: Move rcu_scale_*() after
> > kfree_scale_cleanup()
> >
> > On Wed, May 10, 2023 at 10:12 AM Paul E. McKenney <[email protected]>
> > wrote:
> > >
> > > From: Qiuxu Zhuo <[email protected]>
> > >
> > > This code-movement-only commit moves the rcu_scale_cleanup() and
> > > rcu_scale_shutdown() functions to follow kfree_scale_cleanup().
> > > This is code movement is in preparation for a bug-fix patch that
> > > invokes
> > > kfree_scale_cleanup() from rcu_scale_cleanup().
> > >
> > > Signed-off-by: Qiuxu Zhuo <[email protected]>
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > ---
> > > kernel/rcu/rcuscale.c | 194
> > > +++++++++++++++++++++---------------------
> > > 1 file changed, 97 insertions(+), 97 deletions(-)
> >
> > I wish diff was better at showing what really changed. The meld tool can help
> > but its gui...
> >
> > Should I run meld later (I'm out at a conference so no access to meld-capable
> > machines) or are we sufficiently confident that the lines were moved as-is ? :)
> >
>
> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> I just run the command below and confirmed that the lines were moved
> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> You may double check it ;-).
>
> meld --diff ./rcuscale.c.before ./rcuscale.c.after
Nice, thank you both!
Another option is to check out the commit corresponding to this patch,
then do "git blame -M kernel/rcu/rcuscale.c". Given a move-only commit,
there should be no line tagged with this commit's SHA-1.
They say that another option is to use "git diff --color-moved", which
colors the changes. That it does, but I am hard-pressed to work out
exactly what distinguishes a moved hunk from an added or removed hunk.
Fall colors vs. winter colors? Exterior vs. interior? Any particular
decade in the endless rush of changes to fashion? Perhaps someone with
normal color vision (to say nothing of better fashion sense) could try it.
On the other hand: "default: Is a synonym for zebra. This may change to
a more sensible mode in the future." So maybe it is not just me. ;-)
You can also apparently choose colors using "color.diff.newMoved" and
"color.diff.oldMoved" when using "--color-moved=plain".
But "git diff --color-moved=dimmed-zebra" might be more to the point for
someone like me. I would need to experiment with it more in order to
confirm my hypotheses about what it is doing. To say nothing of building
trust in it. Plus I have to open a color terminal to use it effectively.
So maybe "git blame -M" continues to be the tool for me?
Thanx, Paul
> From: Paul E. McKenney <[email protected]>
> ...
> > > I wish diff was better at showing what really changed. The meld tool
> > > can help but its gui...
> > >
> > > Should I run meld later (I'm out at a conference so no access to
> > > meld-capable
> > > machines) or are we sufficiently confident that the lines were moved
> > > as-is ? :)
> > >
> >
> > Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> > I just run the command below and confirmed that the lines were moved
> > as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> > You may double check it ;-).
> >
> > meld --diff ./rcuscale.c.before ./rcuscale.c.after
>
> Nice, thank you both!
>
> Another option is to check out the commit corresponding to this patch, then
> do "git blame -M kernel/rcu/rcuscale.c". Given a move-only commit, there
> should be no line tagged with this commit's SHA-1.
Just had a good experiment with the "git blame -M" option:
- Used this option to prove a move-only commit quickly (no line tagged with that commit) (the fastest method to me).
- Then just only needed to quickly check the positions of the moved code chunk by myself (easy).
Thank you, Paul for sharing this. It's very useful to me.
> They say that another option is to use "git diff --color-moved", which colors
> the changes. That it does, but I am hard-pressed to work out exactly what
> distinguishes a moved hunk from an added or removed hunk.
> Fall colors vs. winter colors? Exterior vs. interior? Any particular decade in
> the endless rush of changes to fashion? Perhaps someone with normal color
> vision (to say nothing of better fashion sense) could try it.
>
> On the other hand: "default: Is a synonym for zebra. This may change to a
> more sensible mode in the future." So maybe it is not just me. ;-)
>
> You can also apparently choose colors using "color.diff.newMoved" and
> "color.diff.oldMoved" when using "--color-moved=plain".
>
> But "git diff --color-moved=dimmed-zebra" might be more to the point for
> someone like me. I would need to experiment with it more in order to
> confirm my hypotheses about what it is doing. To say nothing of building
Yup, this looks a bit painful for me too (need experiments to confirm hypotheses ...).
> trust in it. Plus I have to open a color terminal to use it effectively.
> So maybe "git blame -M" continues to be the tool for me?
>
> Thanx, Paul
> On May 11, 2023, at 8:20 PM, Zhuo, Qiuxu <[email protected]> wrote:
>
>
>>
>> From: Paul E. McKenney <[email protected]>
>> ...
>>>> I wish diff was better at showing what really changed. The meld tool
>>>> can help but its gui...
>>>>
>>>> Should I run meld later (I'm out at a conference so no access to
>>>> meld-capable
>>>> machines) or are we sufficiently confident that the lines were moved
>>>> as-is ? :)
>>>>
>>>
>>> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
>>> I just run the command below and confirmed that the lines were moved
>>> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
>>> You may double check it ;-).
>>>
>>> meld --diff ./rcuscale.c.before ./rcuscale.c.after
>>
>> Nice, thank you both!
>>
>> Another option is to check out the commit corresponding to this patch, then
>> do "git blame -M kernel/rcu/rcuscale.c". Given a move-only commit, there
>> should be no line tagged with this commit's SHA-1.
>
> Just had a good experiment with the "git blame -M" option:
> - Used this option to prove a move-only commit quickly (no line tagged with that commit) (the fastest method to me).
> - Then just only needed to quickly check the positions of the moved code chunk by myself (easy).
>
> Thank you, Paul for sharing this. It's very useful to me.
Looks good to me as well and thank you both for sharing the tips.
- Joel
>
>> They say that another option is to use "git diff --color-moved", which colors
>> the changes. That it does, but I am hard-pressed to work out exactly what
>> distinguishes a moved hunk from an added or removed hunk.
>> Fall colors vs. winter colors? Exterior vs. interior? Any particular decade in
>> the endless rush of changes to fashion? Perhaps someone with normal color
>> vision (to say nothing of better fashion sense) could try it.
>>
>> On the other hand: "default: Is a synonym for zebra. This may change to a
>> more sensible mode in the future." So maybe it is not just me. ;-)
>>
>> You can also apparently choose colors using "color.diff.newMoved" and
>> "color.diff.oldMoved" when using "--color-moved=plain".
>>
>> But "git diff --color-moved=dimmed-zebra" might be more to the point for
>> someone like me. I would need to experiment with it more in order to
>> confirm my hypotheses about what it is doing. To say nothing of building
>
> Yup, this looks a bit painful for me too (need experiments to confirm hypotheses ...).
>
>> trust in it. Plus I have to open a color terminal to use it effectively.
>> So maybe "git blame -M" continues to be the tool for me?
>>
>> Thanx, Paul
On Thu, May 11, 2023 at 09:15:17PM -0700, Joel Fernandes wrote:
>
>
> > On May 11, 2023, at 8:20 PM, Zhuo, Qiuxu <[email protected]> wrote:
> >
> >
> >>
> >> From: Paul E. McKenney <[email protected]>
> >> ...
> >>>> I wish diff was better at showing what really changed. The meld tool
> >>>> can help but its gui...
> >>>>
> >>>> Should I run meld later (I'm out at a conference so no access to
> >>>> meld-capable
> >>>> machines) or are we sufficiently confident that the lines were moved
> >>>> as-is ? :)
> >>>>
> >>>
> >>> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> >>> I just run the command below and confirmed that the lines were moved
> >>> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> >>> You may double check it ;-).
> >>>
> >>> meld --diff ./rcuscale.c.before ./rcuscale.c.after
> >>
> >> Nice, thank you both!
> >>
> >> Another option is to check out the commit corresponding to this patch, then
> >> do "git blame -M kernel/rcu/rcuscale.c". Given a move-only commit, there
> >> should be no line tagged with this commit's SHA-1.
> >
> > Just had a good experiment with the "git blame -M" option:
> > - Used this option to prove a move-only commit quickly (no line tagged with that commit) (the fastest method to me).
> > - Then just only needed to quickly check the positions of the moved code chunk by myself (easy).
> >
> > Thank you, Paul for sharing this. It's very useful to me.
>
> Looks good to me as well and thank you both for sharing the tips.
Here is one way to script this, where "SHA" identifies the commit to
be checked and PATHS the affected pathnames:
git checkout SHA^
git show SHA | git apply -
git blame -M PATHS | grep '^0* '
If there is no output, there were no non-move changes.
Or just do the "git blame -M PATHS | grep '^0* '" before doing the
checking.
And yes, you can derive PATHS using "git status" if you want. ;-)
Thanx, Paul
> From: Paul E. McKenney <[email protected]>
> ...
> > >>>> I wish diff was better at showing what really changed. The meld
> > >>>> tool can help but its gui...
> > >>>>
> > >>>> Should I run meld later (I'm out at a conference so no access to
> > >>>> meld-capable
> > >>>> machines) or are we sufficiently confident that the lines were
> > >>>> moved as-is ? :)
> > >>>>
> > >>>
> > >>> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> > >>> I just run the command below and confirmed that the lines were
> > >>> moved
> > >>> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> > >>> You may double check it ;-).
> > >>>
> > >>> meld --diff ./rcuscale.c.before ./rcuscale.c.after
> > >>
> > >> Nice, thank you both!
> > >>
> > >> Another option is to check out the commit corresponding to this
> > >> patch, then do "git blame -M kernel/rcu/rcuscale.c". Given a
> > >> move-only commit, there should be no line tagged with this commit's
> SHA-1.
> > >
> > > Just had a good experiment with the "git blame -M" option:
> > > - Used this option to prove a move-only commit quickly (no line tagged
> with that commit) (the fastest method to me).
> > > - Then just only needed to quickly check the positions of the moved code
> chunk by myself (easy).
> > >
> > > Thank you, Paul for sharing this. It's very useful to me.
> >
> > Looks good to me as well and thank you both for sharing the tips.
>
> Here is one way to script this, where "SHA" identifies the commit to be
> checked and PATHS the affected pathnames:
>
> git checkout SHA^
> git show SHA | git apply -
> git blame -M PATHS | grep '^0* '
Cool ~. Thank you, Paul.
I took them and made them into a script below for future use ;-)
#!/bin/bash
SHA=$1
if [ -z "$SHA" ]; then
echo "Usage: $0 <commit-id>"
exit 1
fi
if ! git cat-file -t "$SHA" &> /dev/null; then
echo "$SHA does not exist in the repository"
exit 1
fi
git checkout ${SHA}^ &> /dev/null
git show ${SHA} | git apply - &> /dev/null
PATHS=`git status| grep "modified:" | cut -d: -f2 | xargs`
for P in ${PATHS}; do
R=`git blame -M $P | grep '^0* '`
if test -n "$R"; then
echo "$SHA is NOT a move-only commit"
exit 1
fi
done
echo "$SHA is a move-only commit"
> If there is no output, there were no non-move changes.
>
> Or just do the "git blame -M PATHS | grep '^0* '" before doing the checking.
>
> And yes, you can derive PATHS using "git status" if you want. ;-)
> Thanx, Paul
On Sat, May 13, 2023 at 09:52:46AM +0000, Zhuo, Qiuxu wrote:
> > From: Paul E. McKenney <[email protected]>
> > ...
> > > >>>> I wish diff was better at showing what really changed. The meld
> > > >>>> tool can help but its gui...
> > > >>>>
> > > >>>> Should I run meld later (I'm out at a conference so no access to
> > > >>>> meld-capable
> > > >>>> machines) or are we sufficiently confident that the lines were
> > > >>>> moved as-is ? :)
> > > >>>>
> > > >>>
> > > >>> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> > > >>> I just run the command below and confirmed that the lines were
> > > >>> moved
> > > >>> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> > > >>> You may double check it ;-).
> > > >>>
> > > >>> meld --diff ./rcuscale.c.before ./rcuscale.c.after
> > > >>
> > > >> Nice, thank you both!
> > > >>
> > > >> Another option is to check out the commit corresponding to this
> > > >> patch, then do "git blame -M kernel/rcu/rcuscale.c". Given a
> > > >> move-only commit, there should be no line tagged with this commit's
> > SHA-1.
> > > >
> > > > Just had a good experiment with the "git blame -M" option:
> > > > - Used this option to prove a move-only commit quickly (no line tagged
> > with that commit) (the fastest method to me).
> > > > - Then just only needed to quickly check the positions of the moved code
> > chunk by myself (easy).
> > > >
> > > > Thank you, Paul for sharing this. It's very useful to me.
> > >
> > > Looks good to me as well and thank you both for sharing the tips.
> >
> > Here is one way to script this, where "SHA" identifies the commit to be
> > checked and PATHS the affected pathnames:
> >
> > git checkout SHA^
> > git show SHA | git apply -
> > git blame -M PATHS | grep '^0* '
>
> Cool ~. Thank you, Paul.
> I took them and made them into a script below for future use ;-)
Nice!!!
> #!/bin/bash
>
> SHA=$1
>
> if [ -z "$SHA" ]; then
> echo "Usage: $0 <commit-id>"
> exit 1
> fi
>
> if ! git cat-file -t "$SHA" &> /dev/null; then
> echo "$SHA does not exist in the repository"
> exit 1
> fi
You might want to record the current position so that you can return
to it automatically. One approach is to parse the output of
"git status".
> git checkout ${SHA}^ &> /dev/null
> git show ${SHA} | git apply - &> /dev/null
>
> PATHS=`git status| grep "modified:" | cut -d: -f2 | xargs`
The '--porcelain' argument makes 'git status' is a bit easier to parse
robustly.
> for P in ${PATHS}; do
> R=`git blame -M $P | grep '^0* '`
You can avoid any bash-variable length limitations by using
'grep -q' and capturing the exit status using "$?".
Thanx, Paul
> if test -n "$R"; then
> echo "$SHA is NOT a move-only commit"
> exit 1
> fi
> done
>
> echo "$SHA is a move-only commit"
>
> > If there is no output, there were no non-move changes.
> >
> > Or just do the "git blame -M PATHS | grep '^0* '" before doing the checking.
> >
> > And yes, you can derive PATHS using "git status" if you want. ;-)
> > Thanx, Paul
> From: Paul E. McKenney <[email protected]>
> ...
> > > Here is one way to script this, where "SHA" identifies the commit to
> > > be checked and PATHS the affected pathnames:
> > >
> > > git checkout SHA^
> > > git show SHA | git apply -
> > > git blame -M PATHS | grep '^0* '
> >
> > Cool ~. Thank you, Paul.
> > I took them and made them into a script below for future use ;-)
>
> Nice!!!
>
> > #!/bin/bash
> >
> > SHA=$1
> >
> > if [ -z "$SHA" ]; then
> > echo "Usage: $0 <commit-id>"
> > exit 1
> > fi
> >
> > if ! git cat-file -t "$SHA" &> /dev/null; then
> > echo "$SHA does not exist in the repository"
> > exit 1
> > fi
>
> You might want to record the current position so that you can return to it
> automatically. One approach is to parse the output of "git status".
>
> > git checkout ${SHA}^ &> /dev/null
> > git show ${SHA} | git apply - &> /dev/null
> >
> > PATHS=`git status| grep "modified:" | cut -d: -f2 | xargs`
>
> The '--porcelain' argument makes 'git status' is a bit easier to parse robustly.
>
> > for P in ${PATHS}; do
> > R=`git blame -M $P | grep '^0* '`
>
> You can avoid any bash-variable length limitations by using 'grep -q' and
> capturing the exit status using "$?".
>
Thank you, Paul, for all the enhancement suggestions. ;-)
> Thanx, Paul
>
> ...