* [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test @ 2019-12-19 21:13 Joel Fernandes (Google) 2019-12-21 0:07 ` Paul E. McKenney 0 siblings, 1 reply; 10+ messages in thread From: Joel Fernandes (Google) @ 2019-12-19 21:13 UTC (permalink / raw) To: linux-kernel Cc: Joel Fernandes (Google), bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, Paul E. McKenney, rcu, Steven Rostedt During changes to kfree_rcu() code, we often check the amount of free memory. As an alternative to checking this manually, this commit adds a measurement in the test itself. It measures four times during the test for available memory, digitally filters these measurements to produce a running average with a weight of 0.5, and compares this digitally filtered value with the amount of available memory at the beginning of the test. We apply the digital filter only once we are more than 25% into the test. At the 25% mark, we just read available memory and don't apply any filtering. This prevents the first sample from skewing the results as we would not consider memory readings that were before memory was allocated. A sample run shows something like: Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- v1->v2: Minor corrections v1->v3: Use long long to prevent 32-bit system's overflow Handle case where some threads start later than others. Start measuring only once 25% into the test. Slightly more accurate. Cc: bristot@redhat.com Cc: frextrite@gmail.com Cc: madhuparnabhowmik04@gmail.com Cc: urezki@gmail.com Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c index da94b89cd531..67e0f804ea97 100644 --- a/kernel/rcu/rcuperf.c +++ b/kernel/rcu/rcuperf.c @@ -12,6 +12,7 @@ #include <linux/types.h> #include <linux/kernel.h> #include <linux/init.h> +#include <linux/mm.h> #include <linux/module.h> #include <linux/kthread.h> #include <linux/err.h> @@ -604,6 +605,8 @@ struct kfree_obj { struct rcu_head rh; }; +long long mem_begin; + static int kfree_perf_thread(void *arg) { @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) long me = (long)arg; struct kfree_obj *alloc_ptr; u64 start_time, end_time; + long long mem_during = si_mem_available(); VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) } do { + // Moving average of memory availability measurements. + // Start measuring only from when we are at least 25% into the test. + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { + if (loop == (kfree_loops / 4)) + mem_during = si_mem_available(); + else + mem_during = (mem_during + si_mem_available()) / 2; + } + for (i = 0; i < kfree_alloc_num; i++) { alloc_ptr = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL); if (!alloc_ptr) @@ -645,9 +658,13 @@ kfree_perf_thread(void *arg) else b_rcu_gp_test_finished = cur_ops->get_gp_seq(); - pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n", + // The "memory footprint" field represents how much in-flight + // memory is allocated during the test and waiting to be freed. + pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld, memory footprint: %lldMB\n", (unsigned long long)(end_time - start_time), kfree_loops, - rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started)); + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started), + (mem_begin - mem_during) >> (20 - PAGE_SHIFT)); + if (shutdown) { smp_mb(); /* Assign before wake. */ wake_up(&shutdown_wq); @@ -719,6 +736,8 @@ kfree_perf_init(void) goto unwind; } + mem_begin = si_mem_available(); + for (i = 0; i < kfree_nrealthreads; i++) { firsterr = torture_create_kthread(kfree_perf_thread, (void *)i, kfree_reader_tasks[i]); -- 2.24.1.735.g03f4e72817-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2019-12-19 21:13 [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test Joel Fernandes (Google) @ 2019-12-21 0:07 ` Paul E. McKenney 2019-12-21 3:37 ` Joel Fernandes 0 siblings, 1 reply; 10+ messages in thread From: Paul E. McKenney @ 2019-12-21 0:07 UTC (permalink / raw) To: Joel Fernandes (Google) Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Thu, Dec 19, 2019 at 04:13:49PM -0500, Joel Fernandes (Google) wrote: > During changes to kfree_rcu() code, we often check the amount of free > memory. As an alternative to checking this manually, this commit adds a > measurement in the test itself. It measures four times during the test > for available memory, digitally filters these measurements to produce a > running average with a weight of 0.5, and compares this digitally > filtered value with the amount of available memory at the beginning of > the test. > > We apply the digital filter only once we are more than 25% into the > test. At the 25% mark, we just read available memory and don't apply any > filtering. This prevents the first sample from skewing the results > as we would not consider memory readings that were before memory was > allocated. > > A sample run shows something like: > > Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> Much better! A few comments below. Thanx, Paul > --- > v1->v2: Minor corrections > v1->v3: Use long long to prevent 32-bit system's overflow > Handle case where some threads start later than others. > Start measuring only once 25% into the test. Slightly more accurate. > > Cc: bristot@redhat.com > Cc: frextrite@gmail.com > Cc: madhuparnabhowmik04@gmail.com > Cc: urezki@gmail.com > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c > index da94b89cd531..67e0f804ea97 100644 > --- a/kernel/rcu/rcuperf.c > +++ b/kernel/rcu/rcuperf.c > @@ -12,6 +12,7 @@ > #include <linux/types.h> > #include <linux/kernel.h> > #include <linux/init.h> > +#include <linux/mm.h> > #include <linux/module.h> > #include <linux/kthread.h> > #include <linux/err.h> > @@ -604,6 +605,8 @@ struct kfree_obj { > struct rcu_head rh; > }; > > +long long mem_begin; > + > static int > kfree_perf_thread(void *arg) > { > @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) > long me = (long)arg; > struct kfree_obj *alloc_ptr; > u64 start_time, end_time; > + long long mem_during = si_mem_available(); You initialize here, which makes quite a bit of sense... > VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); > set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); > @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) > } > > do { > + // Moving average of memory availability measurements. > + // Start measuring only from when we are at least 25% into the test. > + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { > + if (loop == (kfree_loops / 4)) > + mem_during = si_mem_available(); But then you reinitialize here. Perhaps to avoid the compiler being confused into complaining about uninitialized variables? (But if so, please comment it.) The thing is that by the fourth measurement, the initial influence has been diluted by a factor of something like 16 or 32, correct? I don't believe that your measurements are any more accurate than that, given the bursty nature of the RCU reclamation process. So why not just initialize it and average it at each sample point? If you want more accuracy, you could increase the number of sample points, while changing the filter constants to more heavily weight past measurements. Actually, I strongly suggest recording frequent raw data, and applying various filtering techniques offline to see what works best. I might be mistaken, but it feels like you are shooting in the dark here. > + else > + mem_during = (mem_during + si_mem_available()) / 2; > + } > + > for (i = 0; i < kfree_alloc_num; i++) { > alloc_ptr = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL); > if (!alloc_ptr) > @@ -645,9 +658,13 @@ kfree_perf_thread(void *arg) > else > b_rcu_gp_test_finished = cur_ops->get_gp_seq(); > > - pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n", > + // The "memory footprint" field represents how much in-flight > + // memory is allocated during the test and waiting to be freed. This added comment is much better, thank you! > + pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld, memory footprint: %lldMB\n", > (unsigned long long)(end_time - start_time), kfree_loops, > - rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started)); > + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started), > + (mem_begin - mem_during) >> (20 - PAGE_SHIFT)); > + > if (shutdown) { > smp_mb(); /* Assign before wake. */ > wake_up(&shutdown_wq); > @@ -719,6 +736,8 @@ kfree_perf_init(void) > goto unwind; > } > > + mem_begin = si_mem_available(); > + > for (i = 0; i < kfree_nrealthreads; i++) { > firsterr = torture_create_kthread(kfree_perf_thread, (void *)i, > kfree_reader_tasks[i]); > -- > 2.24.1.735.g03f4e72817-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2019-12-21 0:07 ` Paul E. McKenney @ 2019-12-21 3:37 ` Joel Fernandes 2020-01-06 19:52 ` Paul E. McKenney 0 siblings, 1 reply; 10+ messages in thread From: Joel Fernandes @ 2019-12-21 3:37 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Fri, Dec 20, 2019 at 04:07:29PM -0800, Paul E. McKenney wrote: > On Thu, Dec 19, 2019 at 04:13:49PM -0500, Joel Fernandes (Google) wrote: > > During changes to kfree_rcu() code, we often check the amount of free > > memory. As an alternative to checking this manually, this commit adds a > > measurement in the test itself. It measures four times during the test > > for available memory, digitally filters these measurements to produce a > > running average with a weight of 0.5, and compares this digitally > > filtered value with the amount of available memory at the beginning of > > the test. > > > > We apply the digital filter only once we are more than 25% into the > > test. At the 25% mark, we just read available memory and don't apply any > > filtering. This prevents the first sample from skewing the results > > as we would not consider memory readings that were before memory was > > allocated. > > > > A sample run shows something like: > > > > Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Much better! A few comments below. > > Thanx, Paul > > > --- > > v1->v2: Minor corrections > > v1->v3: Use long long to prevent 32-bit system's overflow > > Handle case where some threads start later than others. > > Start measuring only once 25% into the test. Slightly more accurate. > > > > Cc: bristot@redhat.com > > Cc: frextrite@gmail.com > > Cc: madhuparnabhowmik04@gmail.com > > Cc: urezki@gmail.com > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c > > index da94b89cd531..67e0f804ea97 100644 > > --- a/kernel/rcu/rcuperf.c > > +++ b/kernel/rcu/rcuperf.c > > @@ -12,6 +12,7 @@ > > #include <linux/types.h> > > #include <linux/kernel.h> > > #include <linux/init.h> > > +#include <linux/mm.h> > > #include <linux/module.h> > > #include <linux/kthread.h> > > #include <linux/err.h> > > @@ -604,6 +605,8 @@ struct kfree_obj { > > struct rcu_head rh; > > }; > > > > +long long mem_begin; > > + > > static int > > kfree_perf_thread(void *arg) > > { > > @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) > > long me = (long)arg; > > struct kfree_obj *alloc_ptr; > > u64 start_time, end_time; > > + long long mem_during = si_mem_available(); > > You initialize here, which makes quite a bit of sense... > > > VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); > > set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); > > @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) > > } > > > > do { > > + // Moving average of memory availability measurements. > > + // Start measuring only from when we are at least 25% into the test. > > + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { > > + if (loop == (kfree_loops / 4)) > > + mem_during = si_mem_available(); > > But then you reinitialize here. Perhaps to avoid the compiler being > confused into complaining about uninitialized variables? (But if so, > please comment it.) It is reinitialized here like that, because if kfree_loops is < 4, then mem_during needs to hold some value to avoid the (mem_begin - mem_during) to falsely appear quite large. That's why I initialized it in the beginning. If kfree_loops is >= 4, then yes it will be initialized twice but that's Ok. I can add a comment to the earlier initialization if you like. If that is the only comment on the patch left, I appreciate if you can change: + long long mem_during = si_mem_available(); to + long long mem_during = si_mem_available(); // for kfree_loops < 4 case > The thing is that by the fourth measurement, the initial influence has > been diluted by a factor of something like 16 or 32, correct? I don't > believe that your measurements are any more accurate than that, given the > bursty nature of the RCU reclamation process. So why not just initialize > it and average it at each sample point? Yes but diluting 200MB of delta by 16 is still high and causes a skew. > > If you want more accuracy, you could increase the number of sample > points, while changing the filter constants to more heavily weight > past measurements. At the moment the results are showing me consistent numbers in my tests and I'm quite happy with them. And it is better to have some measurement, than not having anything. We can certainly refine it further but at this time I am thinking of spending my time reviewing Lai's patches and learning some other RCU things I need to catch up on. If you hate this patch too much, we can also defer this patch review for a bit and I can carry it in my tree for now as it is only a patch to test code. But honestly, in its current form I am sort of happy with it. > Actually, I strongly suggest recording frequent raw data, and applying > various filtering techniques offline to see what works best. I might > be mistaken, but it feels like you are shooting in the dark here. The number I am aiming for is close to a constant. For example in a run, I see the available memory goes down from 460000 pages to ~ 400000 pages. That is around ~240MB of memory. This deviates by +- 20MB. I would rather take many samples than just 1 sample and apply some form of reduction to combine them into a single sample. My current filtering works well in my testing and I could do more complex things, but then again at the moment I am happy with the test. I am open to any other filtering related suggestions though! Sorry if I missed something! thanks, - Joel > > + else > > + mem_during = (mem_during + si_mem_available()) / 2; > > + } > > + > > for (i = 0; i < kfree_alloc_num; i++) { > > alloc_ptr = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL); > > if (!alloc_ptr) > > @@ -645,9 +658,13 @@ kfree_perf_thread(void *arg) > > else > > b_rcu_gp_test_finished = cur_ops->get_gp_seq(); > > > > - pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n", > > + // The "memory footprint" field represents how much in-flight > > + // memory is allocated during the test and waiting to be freed. > > This added comment is much better, thank you! > > > + pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld, memory footprint: %lldMB\n", > > (unsigned long long)(end_time - start_time), kfree_loops, > > - rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started)); > > + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started), > > + (mem_begin - mem_during) >> (20 - PAGE_SHIFT)); > > + > > if (shutdown) { > > smp_mb(); /* Assign before wake. */ > > wake_up(&shutdown_wq); > > @@ -719,6 +736,8 @@ kfree_perf_init(void) > > goto unwind; > > } > > > > + mem_begin = si_mem_available(); > > + > > for (i = 0; i < kfree_nrealthreads; i++) { > > firsterr = torture_create_kthread(kfree_perf_thread, (void *)i, > > kfree_reader_tasks[i]); > > -- > > 2.24.1.735.g03f4e72817-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2019-12-21 3:37 ` Joel Fernandes @ 2020-01-06 19:52 ` Paul E. McKenney 2020-01-15 22:03 ` Joel Fernandes 0 siblings, 1 reply; 10+ messages in thread From: Paul E. McKenney @ 2020-01-06 19:52 UTC (permalink / raw) To: Joel Fernandes Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Fri, Dec 20, 2019 at 10:37:14PM -0500, Joel Fernandes wrote: > On Fri, Dec 20, 2019 at 04:07:29PM -0800, Paul E. McKenney wrote: > > On Thu, Dec 19, 2019 at 04:13:49PM -0500, Joel Fernandes (Google) wrote: > > > During changes to kfree_rcu() code, we often check the amount of free > > > memory. As an alternative to checking this manually, this commit adds a > > > measurement in the test itself. It measures four times during the test > > > for available memory, digitally filters these measurements to produce a > > > running average with a weight of 0.5, and compares this digitally > > > filtered value with the amount of available memory at the beginning of > > > the test. > > > > > > We apply the digital filter only once we are more than 25% into the > > > test. At the 25% mark, we just read available memory and don't apply any > > > filtering. This prevents the first sample from skewing the results > > > as we would not consider memory readings that were before memory was > > > allocated. > > > > > > A sample run shows something like: > > > > > > Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > Much better! A few comments below. > > > > Thanx, Paul > > > > > --- > > > v1->v2: Minor corrections > > > v1->v3: Use long long to prevent 32-bit system's overflow > > > Handle case where some threads start later than others. > > > Start measuring only once 25% into the test. Slightly more accurate. > > > > > > Cc: bristot@redhat.com > > > Cc: frextrite@gmail.com > > > Cc: madhuparnabhowmik04@gmail.com > > > Cc: urezki@gmail.com > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c > > > index da94b89cd531..67e0f804ea97 100644 > > > --- a/kernel/rcu/rcuperf.c > > > +++ b/kernel/rcu/rcuperf.c > > > @@ -12,6 +12,7 @@ > > > #include <linux/types.h> > > > #include <linux/kernel.h> > > > #include <linux/init.h> > > > +#include <linux/mm.h> > > > #include <linux/module.h> > > > #include <linux/kthread.h> > > > #include <linux/err.h> > > > @@ -604,6 +605,8 @@ struct kfree_obj { > > > struct rcu_head rh; > > > }; > > > > > > +long long mem_begin; > > > + > > > static int > > > kfree_perf_thread(void *arg) > > > { > > > @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) > > > long me = (long)arg; > > > struct kfree_obj *alloc_ptr; > > > u64 start_time, end_time; > > > + long long mem_during = si_mem_available(); > > > > You initialize here, which makes quite a bit of sense... > > > > > VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); > > > set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); > > > @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) > > > } > > > > > > do { > > > + // Moving average of memory availability measurements. > > > + // Start measuring only from when we are at least 25% into the test. > > > + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { > > > + if (loop == (kfree_loops / 4)) > > > + mem_during = si_mem_available(); > > > > But then you reinitialize here. Perhaps to avoid the compiler being > > confused into complaining about uninitialized variables? (But if so, > > please comment it.) > > It is reinitialized here like that, because if kfree_loops is < 4, then > mem_during needs to hold some value to avoid the (mem_begin - mem_during) to > falsely appear quite large. That's why I initialized it in the beginning. If > kfree_loops is >= 4, then yes it will be initialized twice but that's Ok. > > I can add a comment to the earlier initialization if you like. Could we just force kfree_loops >= 4? Complain if not, set it to 4? > If that is the only comment on the patch left, I appreciate if you can > change: > + long long mem_during = si_mem_available(); > to > + long long mem_during = si_mem_available(); // for kfree_loops < 4 case Let's see what we end up with. > > The thing is that by the fourth measurement, the initial influence has > > been diluted by a factor of something like 16 or 32, correct? I don't > > believe that your measurements are any more accurate than that, given the > > bursty nature of the RCU reclamation process. So why not just initialize > > it and average it at each sample point? > > Yes but diluting 200MB of delta by 16 is still high and causes a skew. You get similar errors by only sampling four times, though. Assuming a reasonably long test run compared to the typical grace-period duration, each of the four samples has a 50% chance of being above the median, thus a 1/16 chance of all four samples being above the median. > > If you want more accuracy, you could increase the number of sample > > points, while changing the filter constants to more heavily weight > > past measurements. > > At the moment the results are showing me consistent numbers in my tests and > I'm quite happy with them. And it is better to have some measurement, than > not having anything. No argument in general, but consistent numbers do not rule out the possibility of a deterministic bias. > We can certainly refine it further but at this time I am thinking of spending > my time reviewing Lai's patches and learning some other RCU things I need to > catch up on. If you hate this patch too much, we can also defer this patch > review for a bit and I can carry it in my tree for now as it is only a patch > to test code. But honestly, in its current form I am sort of happy with it. OK, I will keep it as is for now and let's look again later on. It is not in the bucket for the upcoming merge window in any case, so we do have quite a bit of time. It is not that I hate it, but rather that I want to be able to give good answers to questions that might come up. And given that I have occasionally given certain people a hard time about their statistics, it is only reasonable to expect them to return the favor. I wouldn't want you to be caught in the crossfire. ;-) > > Actually, I strongly suggest recording frequent raw data, and applying > > various filtering techniques offline to see what works best. I might > > be mistaken, but it feels like you are shooting in the dark here. > > The number I am aiming for is close to a constant. For example in a run, I > see the available memory goes down from 460000 pages to ~ 400000 pages. That > is around ~240MB of memory. This deviates by +- 20MB. I would rather take > many samples than just 1 sample and apply some form of reduction to combine > them into a single sample. My current filtering works well in my testing and > I could do more complex things, but then again at the moment I am happy with > the test. I am open to any other filtering related suggestions though! > > Sorry if I missed something! My thoughts are to use an explicit flag to catch the first-time case, take something like 30 samples as the default, and restructure the loop a bit to reduce at least some of the indentation. But let's keep it on the back burner for a bit while we get other things sorted out. Thanx, Paul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-06 19:52 ` Paul E. McKenney @ 2020-01-15 22:03 ` Joel Fernandes 2020-01-15 22:42 ` Paul E. McKenney 0 siblings, 1 reply; 10+ messages in thread From: Joel Fernandes @ 2020-01-15 22:03 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Mon, Jan 06, 2020 at 11:52:00AM -0800, Paul E. McKenney wrote: > On Fri, Dec 20, 2019 at 10:37:14PM -0500, Joel Fernandes wrote: > > On Fri, Dec 20, 2019 at 04:07:29PM -0800, Paul E. McKenney wrote: > > > On Thu, Dec 19, 2019 at 04:13:49PM -0500, Joel Fernandes (Google) wrote: > > > > During changes to kfree_rcu() code, we often check the amount of free > > > > memory. As an alternative to checking this manually, this commit adds a > > > > measurement in the test itself. It measures four times during the test > > > > for available memory, digitally filters these measurements to produce a > > > > running average with a weight of 0.5, and compares this digitally > > > > filtered value with the amount of available memory at the beginning of > > > > the test. > > > > > > > > We apply the digital filter only once we are more than 25% into the > > > > test. At the 25% mark, we just read available memory and don't apply any > > > > filtering. This prevents the first sample from skewing the results > > > > as we would not consider memory readings that were before memory was > > > > allocated. > > > > > > > > A sample run shows something like: > > > > > > > > Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > Much better! A few comments below. > > > > > > Thanx, Paul > > > > > > > --- > > > > v1->v2: Minor corrections > > > > v1->v3: Use long long to prevent 32-bit system's overflow > > > > Handle case where some threads start later than others. > > > > Start measuring only once 25% into the test. Slightly more accurate. > > > > > > > > Cc: bristot@redhat.com > > > > Cc: frextrite@gmail.com > > > > Cc: madhuparnabhowmik04@gmail.com > > > > Cc: urezki@gmail.com > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- > > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c > > > > index da94b89cd531..67e0f804ea97 100644 > > > > --- a/kernel/rcu/rcuperf.c > > > > +++ b/kernel/rcu/rcuperf.c > > > > @@ -12,6 +12,7 @@ > > > > #include <linux/types.h> > > > > #include <linux/kernel.h> > > > > #include <linux/init.h> > > > > +#include <linux/mm.h> > > > > #include <linux/module.h> > > > > #include <linux/kthread.h> > > > > #include <linux/err.h> > > > > @@ -604,6 +605,8 @@ struct kfree_obj { > > > > struct rcu_head rh; > > > > }; > > > > > > > > +long long mem_begin; > > > > + > > > > static int > > > > kfree_perf_thread(void *arg) > > > > { > > > > @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) > > > > long me = (long)arg; > > > > struct kfree_obj *alloc_ptr; > > > > u64 start_time, end_time; > > > > + long long mem_during = si_mem_available(); > > > > > > You initialize here, which makes quite a bit of sense... > > > > > > > VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); > > > > set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); > > > > @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) > > > > } > > > > > > > > do { > > > > + // Moving average of memory availability measurements. > > > > + // Start measuring only from when we are at least 25% into the test. > > > > + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { > > > > + if (loop == (kfree_loops / 4)) > > > > + mem_during = si_mem_available(); > > > > > > But then you reinitialize here. Perhaps to avoid the compiler being > > > confused into complaining about uninitialized variables? (But if so, > > > please comment it.) > > > > It is reinitialized here like that, because if kfree_loops is < 4, then > > mem_during needs to hold some value to avoid the (mem_begin - mem_during) to > > falsely appear quite large. That's why I initialized it in the beginning. If > > kfree_loops is >= 4, then yes it will be initialized twice but that's Ok. > > > > I can add a comment to the earlier initialization if you like. > > Could we just force kfree_loops >= 4? Complain if not, set it to 4? Sure. > > > The thing is that by the fourth measurement, the initial influence has > > > been diluted by a factor of something like 16 or 32, correct? I don't > > > believe that your measurements are any more accurate than that, given the > > > bursty nature of the RCU reclamation process. So why not just initialize > > > it and average it at each sample point? > > > > Yes but diluting 200MB of delta by 16 is still high and causes a skew. > > You get similar errors by only sampling four times, though. Assuming a > reasonably long test run compared to the typical grace-period duration, > each of the four samples has a 50% chance of being above the median, > thus a 1/16 chance of all four samples being above the median. [snip] > > We can certainly refine it further but at this time I am thinking of spending > > my time reviewing Lai's patches and learning some other RCU things I need to > > catch up on. If you hate this patch too much, we can also defer this patch > > review for a bit and I can carry it in my tree for now as it is only a patch > > to test code. But honestly, in its current form I am sort of happy with it. > > OK, I will keep it as is for now and let's look again later on. It is not > in the bucket for the upcoming merge window in any case, so we do have > quite a bit of time. > > It is not that I hate it, but rather that I want to be able to give > good answers to questions that might come up. And given that I have > occasionally given certain people a hard time about their statistics, > it is only reasonable to expect them to return the favor. I wouldn't > want you to be caught in the crossfire. ;-) Since the weights were concerning, I was thinking of just using a weight of (1 / N) where N is the number of samples. Essentially taking the average. That could be simple enough and does not cause your concerns with weight tuning. I tested it and looks good, I'll post it shortly. thanks, - Joel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-15 22:03 ` Joel Fernandes @ 2020-01-15 22:42 ` Paul E. McKenney 2020-01-15 22:45 ` Joel Fernandes 0 siblings, 1 reply; 10+ messages in thread From: Paul E. McKenney @ 2020-01-15 22:42 UTC (permalink / raw) To: Joel Fernandes Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Wed, Jan 15, 2020 at 05:03:00PM -0500, Joel Fernandes wrote: > On Mon, Jan 06, 2020 at 11:52:00AM -0800, Paul E. McKenney wrote: > > On Fri, Dec 20, 2019 at 10:37:14PM -0500, Joel Fernandes wrote: > > > On Fri, Dec 20, 2019 at 04:07:29PM -0800, Paul E. McKenney wrote: > > > > On Thu, Dec 19, 2019 at 04:13:49PM -0500, Joel Fernandes (Google) wrote: > > > > > During changes to kfree_rcu() code, we often check the amount of free > > > > > memory. As an alternative to checking this manually, this commit adds a > > > > > measurement in the test itself. It measures four times during the test > > > > > for available memory, digitally filters these measurements to produce a > > > > > running average with a weight of 0.5, and compares this digitally > > > > > filtered value with the amount of available memory at the beginning of > > > > > the test. > > > > > > > > > > We apply the digital filter only once we are more than 25% into the > > > > > test. At the 25% mark, we just read available memory and don't apply any > > > > > filtering. This prevents the first sample from skewing the results > > > > > as we would not consider memory readings that were before memory was > > > > > allocated. > > > > > > > > > > A sample run shows something like: > > > > > > > > > > Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > Much better! A few comments below. > > > > > > > > Thanx, Paul > > > > > > > > > --- > > > > > v1->v2: Minor corrections > > > > > v1->v3: Use long long to prevent 32-bit system's overflow > > > > > Handle case where some threads start later than others. > > > > > Start measuring only once 25% into the test. Slightly more accurate. > > > > > > > > > > Cc: bristot@redhat.com > > > > > Cc: frextrite@gmail.com > > > > > Cc: madhuparnabhowmik04@gmail.com > > > > > Cc: urezki@gmail.com > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > kernel/rcu/rcuperf.c | 23 +++++++++++++++++++++-- > > > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c > > > > > index da94b89cd531..67e0f804ea97 100644 > > > > > --- a/kernel/rcu/rcuperf.c > > > > > +++ b/kernel/rcu/rcuperf.c > > > > > @@ -12,6 +12,7 @@ > > > > > #include <linux/types.h> > > > > > #include <linux/kernel.h> > > > > > #include <linux/init.h> > > > > > +#include <linux/mm.h> > > > > > #include <linux/module.h> > > > > > #include <linux/kthread.h> > > > > > #include <linux/err.h> > > > > > @@ -604,6 +605,8 @@ struct kfree_obj { > > > > > struct rcu_head rh; > > > > > }; > > > > > > > > > > +long long mem_begin; > > > > > + > > > > > static int > > > > > kfree_perf_thread(void *arg) > > > > > { > > > > > @@ -611,6 +614,7 @@ kfree_perf_thread(void *arg) > > > > > long me = (long)arg; > > > > > struct kfree_obj *alloc_ptr; > > > > > u64 start_time, end_time; > > > > > + long long mem_during = si_mem_available(); > > > > > > > > You initialize here, which makes quite a bit of sense... > > > > > > > > > VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); > > > > > set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); > > > > > @@ -626,6 +630,15 @@ kfree_perf_thread(void *arg) > > > > > } > > > > > > > > > > do { > > > > > + // Moving average of memory availability measurements. > > > > > + // Start measuring only from when we are at least 25% into the test. > > > > > + if (loop && kfree_loops > 3 && (loop % (kfree_loops / 4) == 0)) { > > > > > + if (loop == (kfree_loops / 4)) > > > > > + mem_during = si_mem_available(); > > > > > > > > But then you reinitialize here. Perhaps to avoid the compiler being > > > > confused into complaining about uninitialized variables? (But if so, > > > > please comment it.) > > > > > > It is reinitialized here like that, because if kfree_loops is < 4, then > > > mem_during needs to hold some value to avoid the (mem_begin - mem_during) to > > > falsely appear quite large. That's why I initialized it in the beginning. If > > > kfree_loops is >= 4, then yes it will be initialized twice but that's Ok. > > > > > > I can add a comment to the earlier initialization if you like. > > > > Could we just force kfree_loops >= 4? Complain if not, set it to 4? > > Sure. > > > > > The thing is that by the fourth measurement, the initial influence has > > > > been diluted by a factor of something like 16 or 32, correct? I don't > > > > believe that your measurements are any more accurate than that, given the > > > > bursty nature of the RCU reclamation process. So why not just initialize > > > > it and average it at each sample point? > > > > > > Yes but diluting 200MB of delta by 16 is still high and causes a skew. > > > > You get similar errors by only sampling four times, though. Assuming a > > reasonably long test run compared to the typical grace-period duration, > > each of the four samples has a 50% chance of being above the median, > > thus a 1/16 chance of all four samples being above the median. > > [snip] > > > We can certainly refine it further but at this time I am thinking of spending > > > my time reviewing Lai's patches and learning some other RCU things I need to > > > catch up on. If you hate this patch too much, we can also defer this patch > > > review for a bit and I can carry it in my tree for now as it is only a patch > > > to test code. But honestly, in its current form I am sort of happy with it. > > > > OK, I will keep it as is for now and let's look again later on. It is not > > in the bucket for the upcoming merge window in any case, so we do have > > quite a bit of time. > > > > It is not that I hate it, but rather that I want to be able to give > > good answers to questions that might come up. And given that I have > > occasionally given certain people a hard time about their statistics, > > it is only reasonable to expect them to return the favor. I wouldn't > > want you to be caught in the crossfire. ;-) > > Since the weights were concerning, I was thinking of just using a weight of > (1 / N) where N is the number of samples. Essentially taking the average. > That could be simple enough and does not cause your concerns with weight > tuning. I tested it and looks good, I'll post it shortly. YES!!! ;-) Snapshot mem_begin before entering the loop. For the mean value to be solid, you need at least 20-30 samples, which might mean upping the default for kfree_loops. Have an "unsigned long long" to accumulate the sum, which should avoid any possibility of overflow for current systems and for all systems for kfree_loops less than PAGE_SIZE. At which point, forget the "%" stuff and just sum up the si_mem_available() on each pass through the loop. Do the division on exit from the loop, preferably checking for divide by zero. Straightforward, fast, reasonably reliable, and easy to defend. Thanx, Paul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-15 22:42 ` Paul E. McKenney @ 2020-01-15 22:45 ` Joel Fernandes 2020-01-16 0:01 ` Paul E. McKenney 0 siblings, 1 reply; 10+ messages in thread From: Joel Fernandes @ 2020-01-15 22:45 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Wed, Jan 15, 2020 at 02:42:51PM -0800, Paul E. McKenney wrote: > > [snip] > > > > We can certainly refine it further but at this time I am thinking of spending > > > > my time reviewing Lai's patches and learning some other RCU things I need to > > > > catch up on. If you hate this patch too much, we can also defer this patch > > > > review for a bit and I can carry it in my tree for now as it is only a patch > > > > to test code. But honestly, in its current form I am sort of happy with it. > > > > > > OK, I will keep it as is for now and let's look again later on. It is not > > > in the bucket for the upcoming merge window in any case, so we do have > > > quite a bit of time. > > > > > > It is not that I hate it, but rather that I want to be able to give > > > good answers to questions that might come up. And given that I have > > > occasionally given certain people a hard time about their statistics, > > > it is only reasonable to expect them to return the favor. I wouldn't > > > want you to be caught in the crossfire. ;-) > > > > Since the weights were concerning, I was thinking of just using a weight of > > (1 / N) where N is the number of samples. Essentially taking the average. > > That could be simple enough and does not cause your concerns with weight > > tuning. I tested it and looks good, I'll post it shortly. > > YES!!! ;-) > > Snapshot mem_begin before entering the loop. For the mean value to > be solid, you need at least 20-30 samples, which might mean upping the > default for kfree_loops. Have an "unsigned long long" to accumulate the > sum, which should avoid any possibility of overflow for current systems > and for all systems for kfree_loops less than PAGE_SIZE. At which point, > forget the "%" stuff and just sum up the si_mem_available() on each pass > through the loop. > > Do the division on exit from the loop, preferably checking for divide > by zero. > > Straightforward, fast, reasonably reliable, and easy to defend. I mostly did it along these lines. Hopefully the latest posting is reasonable enough ;-) I sent it twice because I messed up the authorship (sorry). thanks, - Joel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-15 22:45 ` Joel Fernandes @ 2020-01-16 0:01 ` Paul E. McKenney 2020-01-16 4:05 ` Joel Fernandes 0 siblings, 1 reply; 10+ messages in thread From: Paul E. McKenney @ 2020-01-16 0:01 UTC (permalink / raw) To: Joel Fernandes Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Wed, Jan 15, 2020 at 05:45:42PM -0500, Joel Fernandes wrote: > On Wed, Jan 15, 2020 at 02:42:51PM -0800, Paul E. McKenney wrote: > > > [snip] > > > > > We can certainly refine it further but at this time I am thinking of spending > > > > > my time reviewing Lai's patches and learning some other RCU things I need to > > > > > catch up on. If you hate this patch too much, we can also defer this patch > > > > > review for a bit and I can carry it in my tree for now as it is only a patch > > > > > to test code. But honestly, in its current form I am sort of happy with it. > > > > > > > > OK, I will keep it as is for now and let's look again later on. It is not > > > > in the bucket for the upcoming merge window in any case, so we do have > > > > quite a bit of time. > > > > > > > > It is not that I hate it, but rather that I want to be able to give > > > > good answers to questions that might come up. And given that I have > > > > occasionally given certain people a hard time about their statistics, > > > > it is only reasonable to expect them to return the favor. I wouldn't > > > > want you to be caught in the crossfire. ;-) > > > > > > Since the weights were concerning, I was thinking of just using a weight of > > > (1 / N) where N is the number of samples. Essentially taking the average. > > > That could be simple enough and does not cause your concerns with weight > > > tuning. I tested it and looks good, I'll post it shortly. > > > > YES!!! ;-) > > > > Snapshot mem_begin before entering the loop. For the mean value to > > be solid, you need at least 20-30 samples, which might mean upping the > > default for kfree_loops. Have an "unsigned long long" to accumulate the > > sum, which should avoid any possibility of overflow for current systems > > and for all systems for kfree_loops less than PAGE_SIZE. At which point, > > forget the "%" stuff and just sum up the si_mem_available() on each pass > > through the loop. > > > > Do the division on exit from the loop, preferably checking for divide > > by zero. > > > > Straightforward, fast, reasonably reliable, and easy to defend. > > I mostly did it along these lines. Hopefully the latest posting is reasonable > enough ;-) I sent it twice because I messed up the authorship (sorry). No problem with the authorship-fix resend! But let's get this patch consistent with basic statistics! You really do need 20-30 samples for an average to mean much. Of course, right now you default kfree_loops to 10. You are doing 8000 kmalloc()/kfree_rcu() loops on each pass. This is large enough that just dropping the "% 4" should be just fine from the viewpoint of si_mem_available() overhead. But 8000 allocations and frees should get done in way less than one second, so kicking the default kfree_loops up to 30 should be a non-problem. Then the patch would be both simpler and statistically valid. So could you please stop using me as the middleman in your fight with the laws of mathematics and get this patch to a defensible state? ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-16 0:01 ` Paul E. McKenney @ 2020-01-16 4:05 ` Joel Fernandes 2020-01-16 19:25 ` Joel Fernandes 0 siblings, 1 reply; 10+ messages in thread From: Joel Fernandes @ 2020-01-16 4:05 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Wed, Jan 15, 2020 at 04:01:04PM -0800, Paul E. McKenney wrote: > On Wed, Jan 15, 2020 at 05:45:42PM -0500, Joel Fernandes wrote: > > On Wed, Jan 15, 2020 at 02:42:51PM -0800, Paul E. McKenney wrote: > > > > [snip] > > > > > > We can certainly refine it further but at this time I am thinking of spending > > > > > > my time reviewing Lai's patches and learning some other RCU things I need to > > > > > > catch up on. If you hate this patch too much, we can also defer this patch > > > > > > review for a bit and I can carry it in my tree for now as it is only a patch > > > > > > to test code. But honestly, in its current form I am sort of happy with it. > > > > > > > > > > OK, I will keep it as is for now and let's look again later on. It is not > > > > > in the bucket for the upcoming merge window in any case, so we do have > > > > > quite a bit of time. > > > > > > > > > > It is not that I hate it, but rather that I want to be able to give > > > > > good answers to questions that might come up. And given that I have > > > > > occasionally given certain people a hard time about their statistics, > > > > > it is only reasonable to expect them to return the favor. I wouldn't > > > > > want you to be caught in the crossfire. ;-) > > > > > > > > Since the weights were concerning, I was thinking of just using a weight of > > > > (1 / N) where N is the number of samples. Essentially taking the average. > > > > That could be simple enough and does not cause your concerns with weight > > > > tuning. I tested it and looks good, I'll post it shortly. > > > > > > YES!!! ;-) > > > > > > Snapshot mem_begin before entering the loop. For the mean value to > > > be solid, you need at least 20-30 samples, which might mean upping the > > > default for kfree_loops. Have an "unsigned long long" to accumulate the > > > sum, which should avoid any possibility of overflow for current systems > > > and for all systems for kfree_loops less than PAGE_SIZE. At which point, > > > forget the "%" stuff and just sum up the si_mem_available() on each pass > > > through the loop. > > > > > > Do the division on exit from the loop, preferably checking for divide > > > by zero. > > > > > > Straightforward, fast, reasonably reliable, and easy to defend. > > > > I mostly did it along these lines. Hopefully the latest posting is reasonable > > enough ;-) I sent it twice because I messed up the authorship (sorry). > > No problem with the authorship-fix resend! > > But let's get this patch consistent with basic statistics! > > You really do need 20-30 samples for an average to mean much. > > Of course, right now you default kfree_loops to 10. You are doing > 8000 kmalloc()/kfree_rcu() loops on each pass. This is large enough > that just dropping the "% 4" should be just fine from the viewpoint of > si_mem_available() overhead. But 8000 allocations and frees should get > done in way less than one second, so kicking the default kfree_loops up > to 30 should be a non-problem. > > Then the patch would be both simpler and statistically valid. > > So could you please stop using me as the middleman in your fight with > the laws of mathematics and get this patch to a defensible state? ;-) The thing is the signal doesn't vary much. I could very well just take one out of the 4 samples and report that. But I still took the average since there are 4 samples. I don't see much point in taking more samples here since I am not concerned that the signal will fluctuate much (and if it really does, then I can easily catch that kind of variation with multiple rcuperf runs). But if you really want though, I can increase the sampling to 20 samples or a number like that and resend it. thanks, - Joel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test 2020-01-16 4:05 ` Joel Fernandes @ 2020-01-16 19:25 ` Joel Fernandes 0 siblings, 0 replies; 10+ messages in thread From: Joel Fernandes @ 2020-01-16 19:25 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, bristot, frextrite, madhuparnabhowmik04, urezki, Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt On Wed, Jan 15, 2020 at 11:05:58PM -0500, Joel Fernandes wrote: > On Wed, Jan 15, 2020 at 04:01:04PM -0800, Paul E. McKenney wrote: > > On Wed, Jan 15, 2020 at 05:45:42PM -0500, Joel Fernandes wrote: > > > On Wed, Jan 15, 2020 at 02:42:51PM -0800, Paul E. McKenney wrote: > > > > > [snip] > > > > > > > We can certainly refine it further but at this time I am thinking of spending > > > > > > > my time reviewing Lai's patches and learning some other RCU things I need to > > > > > > > catch up on. If you hate this patch too much, we can also defer this patch > > > > > > > review for a bit and I can carry it in my tree for now as it is only a patch > > > > > > > to test code. But honestly, in its current form I am sort of happy with it. > > > > > > > > > > > > OK, I will keep it as is for now and let's look again later on. It is not > > > > > > in the bucket for the upcoming merge window in any case, so we do have > > > > > > quite a bit of time. > > > > > > > > > > > > It is not that I hate it, but rather that I want to be able to give > > > > > > good answers to questions that might come up. And given that I have > > > > > > occasionally given certain people a hard time about their statistics, > > > > > > it is only reasonable to expect them to return the favor. I wouldn't > > > > > > want you to be caught in the crossfire. ;-) > > > > > > > > > > Since the weights were concerning, I was thinking of just using a weight of > > > > > (1 / N) where N is the number of samples. Essentially taking the average. > > > > > That could be simple enough and does not cause your concerns with weight > > > > > tuning. I tested it and looks good, I'll post it shortly. > > > > > > > > YES!!! ;-) > > > > > > > > Snapshot mem_begin before entering the loop. For the mean value to > > > > be solid, you need at least 20-30 samples, which might mean upping the > > > > default for kfree_loops. Have an "unsigned long long" to accumulate the > > > > sum, which should avoid any possibility of overflow for current systems > > > > and for all systems for kfree_loops less than PAGE_SIZE. At which point, > > > > forget the "%" stuff and just sum up the si_mem_available() on each pass > > > > through the loop. > > > > > > > > Do the division on exit from the loop, preferably checking for divide > > > > by zero. > > > > > > > > Straightforward, fast, reasonably reliable, and easy to defend. > > > > > > I mostly did it along these lines. Hopefully the latest posting is reasonable > > > enough ;-) I sent it twice because I messed up the authorship (sorry). > > > > No problem with the authorship-fix resend! > > > > But let's get this patch consistent with basic statistics! > > > > You really do need 20-30 samples for an average to mean much. > > > > Of course, right now you default kfree_loops to 10. You are doing > > 8000 kmalloc()/kfree_rcu() loops on each pass. This is large enough > > that just dropping the "% 4" should be just fine from the viewpoint of > > si_mem_available() overhead. But 8000 allocations and frees should get > > done in way less than one second, so kicking the default kfree_loops up > > to 30 should be a non-problem. > > > > Then the patch would be both simpler and statistically valid. > > > > So could you please stop using me as the middleman in your fight with > > the laws of mathematics and get this patch to a defensible state? ;-) > > The thing is the signal doesn't vary much. I could very well just take one > out of the 4 samples and report that. But I still took the average since > there are 4 samples. I don't see much point in taking more samples here since > I am not concerned that the signal will fluctuate much (and if it really > does, then I can easily catch that kind of variation with multiple rcuperf > runs). > > But if you really want though, I can increase the sampling to 20 samples or a > number like that and resend it. Changed it to around 20 samples and the tests look fine. Updated patch below (v5) : ---8<----------------------- From: "Joel Fernandes (Google)" <joel@joelfernandes.org> Subject: [PATCH] rcuperf: Measure memory footprint during kfree_rcu() test (v5) During changes to kfree_rcu() code, we often check the amount of free memory. As an alternative to checking this manually, this commit adds a measurement in the test itself. It measures four times during the test for available memory, digitally filters these measurements to produce a running average with a weight of 0.5, and compares this digitally filtered value with the amount of available memory at the beginning of the test. We apply the digital filter only once we are more than 25% into the test. At the 25% mark, we just read available memory and don't apply any filtering. This prevents the first sample from skewing the results as we would not consider memory readings that were before memory was allocated. A sample run shows something like: Total time taken by all kfree'ers: 6369738407 ns, loops: 10000, batches: 764, memory footprint: 216MB Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- v1->v2: Minor corrections v1->v3: Use long long to prevent 32-bit system's overflow Handle case where some threads start later than others. Start measuring only once 25% into the test. Slightly more accurate. v3->v4: Simplified test more. Using simple average. v4->v5: Collect more memory samples than previous revision (4 to 5, vs, 20 to 21). kernel/rcu/rcuperf.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c index 1fd0cc72022e..284625af4c79 100644 --- a/kernel/rcu/rcuperf.c +++ b/kernel/rcu/rcuperf.c @@ -12,6 +12,7 @@ #include <linux/types.h> #include <linux/kernel.h> #include <linux/init.h> +#include <linux/mm.h> #include <linux/module.h> #include <linux/kthread.h> #include <linux/err.h> @@ -593,7 +594,7 @@ rcu_perf_shutdown(void *arg) torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu()."); torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration."); -torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees."); +torture_param(int, kfree_loops, 20, "Number of loops doing kfree_alloc_num allocations and frees."); static struct task_struct **kfree_reader_tasks; static int kfree_nrealthreads; @@ -616,14 +617,17 @@ DEFINE_KFREE_OBJ(32); // goes on kmalloc-64 slab DEFINE_KFREE_OBJ(64); // goes on kmalloc-96 slab DEFINE_KFREE_OBJ(96); // goes on kmalloc-128 slab +long long mem_begin; + static int kfree_perf_thread(void *arg) { int i, loop = 0; long me = (long)arg; void *alloc_ptr; - u64 start_time, end_time; + long mem_samples = 0; + long long mem_during = 0; VERBOSE_PERFOUT_STRING("kfree_perf_thread task started"); set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids)); @@ -638,7 +642,17 @@ kfree_perf_thread(void *arg) b_rcu_gp_test_started = cur_ops->get_gp_seq(); } + // Prevent "% 0" error below. + if (kfree_loops < 20) + kfree_loops = 20; + do { + // Start measuring only from when we are a little into the test. + if ((loop != 0) && (loop % (kfree_loops / 20) == 0)) { + mem_during = mem_during + si_mem_available(); + mem_samples++; + } + for (i = 0; i < kfree_alloc_num; i++) { int kfree_type = i % 4; @@ -671,6 +685,8 @@ kfree_perf_thread(void *arg) cond_resched(); } while (!torture_must_stop() && ++loop < kfree_loops); + mem_during = (mem_during / mem_samples); + if (atomic_inc_return(&n_kfree_perf_thread_ended) >= kfree_nrealthreads) { end_time = ktime_get_mono_fast_ns(); @@ -679,9 +695,13 @@ kfree_perf_thread(void *arg) else b_rcu_gp_test_finished = cur_ops->get_gp_seq(); - pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n", + // The "memory footprint" field represents how much in-flight + // memory is allocated during the test and waiting to be freed. + pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld, memory footprint: %lldMB\n", (unsigned long long)(end_time - start_time), kfree_loops, - rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started)); + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started), + (mem_begin - mem_during) >> (20 - PAGE_SHIFT)); + if (shutdown) { smp_mb(); /* Assign before wake. */ wake_up(&shutdown_wq); @@ -753,6 +773,8 @@ kfree_perf_init(void) goto unwind; } + mem_begin = si_mem_available(); + for (i = 0; i < kfree_nrealthreads; i++) { firsterr = torture_create_kthread(kfree_perf_thread, (void *)i, kfree_reader_tasks[i]); -- 2.25.0.rc1.283.g88dfdc4193-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-16 19:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-19 21:13 [PATCH v3 rcu-dev] rcuperf: Measure memory footprint during kfree_rcu() test Joel Fernandes (Google) 2019-12-21 0:07 ` Paul E. McKenney 2019-12-21 3:37 ` Joel Fernandes 2020-01-06 19:52 ` Paul E. McKenney 2020-01-15 22:03 ` Joel Fernandes 2020-01-15 22:42 ` Paul E. McKenney 2020-01-15 22:45 ` Joel Fernandes 2020-01-16 0:01 ` Paul E. McKenney 2020-01-16 4:05 ` Joel Fernandes 2020-01-16 19:25 ` Joel Fernandes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).