On Mon, Aug 19, 2019 at 07:51:23PM -0400, Joel Fernandes wrote:
> On Mon, Aug 19, 2019 at 03:23:30PM -0700, Paul E. McKenney wrote:
> [snip]
> > > [snip]
> > > > > @@ -592,6 +593,175 @@ rcu_perf_shutdown(void *arg)
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * kfree_rcu performance 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.
> > > > > + */
> > > > > +
> > > > > +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_no_batch, 0, "Use the non-batching (slower)
> > > > > version of kfree_rcu.");
> > > > > +
> > > > > +static struct task_struct **kfree_reader_tasks;
> > > > > +static int kfree_nrealthreads;
> > > > > +static atomic_t n_kfree_perf_thread_started;
> > > > > +static atomic_t n_kfree_perf_thread_ended;
> > > > > +
> > > > > +struct kfree_obj {
> > > > > + char kfree_obj[8];
> > > > > + struct rcu_head rh;
> > > > > +};
> > > >
> > > > (Aside from above, no need to change this part of the patch, at least
> > > > not
> > > > that I know of at the moment.)
> > > >
> > > > 24 bytes on a 64-bit system, 16 on a 32-bit system. So there might
> > > > have been 10 million extra objects awaiting free in the batching case
> > > > given the 400M-50M=350M excess for the batching approach. If freeing
> > > > each object took about 100ns, that could account for the additional
> > > > wall-clock time for the batching approach.
> > >
> > > Makes sense, and this comes down to 200-220MB range with the additional
> > > list.
> >
> > Which might even match the observed numbers?
>
> Yes, they would. Since those *are* the observed numbers :-D ;-) ;-)
;-)
> > > > > + do {
> > > > > + for (i = 0; i < kfree_alloc_num; i++) {
> > > > > + alloc_ptrs[i] = kmalloc(sizeof(struct
> > > > > kfree_obj), GFP_KERNEL);
> > > > > + if (!alloc_ptrs[i])
> > > > > + return -ENOMEM;
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < kfree_alloc_num; i++) {
> > > > > + if (!kfree_no_batch) {
> > > > > + kfree_rcu(alloc_ptrs[i], rh);
> > > > > + } else {
> > > > > + rcu_callback_t cb;
> > > > > +
> > > > > + cb = (rcu_callback_t)(unsigned
> > > > > long)offsetof(struct kfree_obj, rh);
> > > > > +
> > > > > kfree_call_rcu_nobatch(&(alloc_ptrs[i]->rh), cb);
> > > > > + }
> > > > > + }
> > > >
> > > > The point of allocating a large batch and then kfree_rcu()ing them in a
> > > > loop is to defeat the per-CPU pool optimization? Either way, a comment
> > > > would be very good!
> > >
> > > It was a reasoning like this, added it as a comment:
> > >
> > > /* While measuring kfree_rcu() time, we also end up measuring kmalloc()
> > > * time. So the strategy here is to do a few (kfree_alloc_num) number
> > > * of kmalloc() and kfree_rcu() every loop so that the current loop's
> > > * deferred kfree()ing overlaps with the next loop's kmalloc().
> > > */
> >
> > The thought being that the CPU will be executing the two loops
> > concurrently? Up to a point, agreed, but how much of an effect is
> > that, really?
>
> Yes it may not matter much. It was just a small thought when I added the
> loop, I had to start somewhere, so I did it this way.
>
> > Or is the idea to time the kfree_rcu() loop separately? (I don't see
> > any such separate timing, though.)
>
> The kmalloc() times are included within the kfree loop. The timing of
> kfree_rcu() is not separate in my patch.
You lost me on this one. What happens when you just interleave the
kmalloc() and kfree_rcu(), without looping, compared to the looping
above? Does this get more expensive? Cheaper? More vulnerable to OOM?
Something else?
Thanx, Paul
