On Tue, Sep 03, 2019 at 01:08:49PM -0700, Paul E. McKenney wrote:
> On Thu, Aug 29, 2019 at 04:56:37PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 28, 2019 at 02:12:26PM -0700, Paul E. McKenney wrote:
>
> [ . . . ]
>
> > > > +static int
> > > > +kfree_perf_thread(void *arg)
> > > > +{
> > > > + int i, loop = 0;
> > > > + long me = (long)arg;
> > > > + struct kfree_obj *alloc_ptr;
> > > > + u64 start_time, end_time;
> > > > +
> > > > + VERBOSE_PERFOUT_STRING("kfree_perf_thread task started");
> > > > + set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
> > > > + set_user_nice(current, MAX_NICE);
> > > > +
> > > > + start_time = ktime_get_mono_fast_ns();
> > > > +
> > > > + if (atomic_inc_return(&n_kfree_perf_thread_started) >= kfree_nrealthreads) {
> > > > + if (gp_exp)
> > > > + b_rcu_gp_test_started = cur_ops->exp_completed() / 2;
> > >
> > > At some point, it would be good to use the new grace-period
> > > sequence-counter functions (rcuperf_seq_diff(), for example) instead of
> > > the open-coded division by 2. I freely admit that you are just copying
> > > my obsolete hack in this case, so not needed in this patch.
> >
> > But I am using rcu_seq_diff() below in the pr_alert().
> >
> > Anyway, I agree this can be a follow-on since this pattern is borrowed from
> > another part of rcuperf. However, I am also confused about the pattern
> > itself.
> >
> > If I understand, you are doing the "/ 2" because expedited_sequence
> > progresses by 2 for every expedited batch.
> >
> > But does rcu_seq_diff() really work on these expedited GP numbers, and will
> > it be immune to changes in RCU_SEQ_STATE_MASK? Sorry for the silly questions,
> > but admittedly I have not looked too much yet into expedited RCU so I could
> > be missing the point.
>
> Yes, expedited grace periods use the common sequence-number functions.
> Oddly enough, normal grace periods were the last to make use of these.
Ok, will clean up in a follow on patch as we agreed, so as to not block this series.
> > > > + 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",
> > > > + (unsigned long long)(end_time - start_time), kfree_loops,
> > > > + rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started));
> > > > + if (shutdown) {
> > > > + smp_mb(); /* Assign before wake. */
> > > > + wake_up(&shutdown_wq);
> > > > + }
> > > > + }
> > > > +
> > > > + torture_kthread_stopping("kfree_perf_thread");
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void
> > > > +kfree_perf_cleanup(void)
> > > > +{
> > > > + int i;
> > > > +
> > > > + if (torture_cleanup_begin())
> > > > + return;
> > > > +
> > > > + if (kfree_reader_tasks) {
> > > > + for (i = 0; i < kfree_nrealthreads; i++)
> > > > + torture_stop_kthread(kfree_perf_thread,
> > > > + kfree_reader_tasks[i]);
> > > > + kfree(kfree_reader_tasks);
> > > > + }
> > > > +
> > > > + torture_cleanup_end();
> > > > +}
> > > > +
> > > > +/*
> > > > + * shutdown kthread. Just waits to be awakened, then shuts down system.
> > > > + */
> > > > +static int
> > > > +kfree_perf_shutdown(void *arg)
> > > > +{
> > > > + do {
> > > > + wait_event(shutdown_wq,
> > > > + atomic_read(&n_kfree_perf_thread_ended) >=
> > > > + kfree_nrealthreads);
> > > > + } while (atomic_read(&n_kfree_perf_thread_ended) < kfree_nrealthreads);
> > > > +
> > > > + smp_mb(); /* Wake before output. */
> > > > +
> > > > + kfree_perf_cleanup();
> > > > + kernel_power_off();
> > > > + return -EINVAL;
> > >
> > > These last four lines should be combined with those of
> > > rcu_perf_shutdown(). Actually, you could fold the two functions together
> > > with only a pair of arguments and two one-line wrapper functions, which
> > > would be even better.
> >
> > But the cleanup() function is different in the 2 cases and will have to be
> > passed in as a function pointer. I believe we discussed this last review as
> > well.
>
> Calling through a pointer should be a non-problem in this case. We are
> nowhere near a fastpath.
There's also the wait_event() condition that is different. I don't see how we
can combine this. It will look though and with probably the same lines of
code. Can this function be as is? pretty-please :-D. Or perhaps, if you feel
it is a trivial cleanup, could you do it so I can understand what you mean?
Sorry!
thanks!
- Joel
