Re: [RFC] [PATCH 0/6] Provide cgroup isolation for buffered writes.

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Mar 15, 2011 at 09:41:50AM -0700, Justin TerAvest wrote:
> On Fri, Mar 11, 2011 at 8:39 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 11, 2011 at 08:07:17AM -0800, Justin TerAvest wrote:
> >> On Thu, Mar 10, 2011 at 6:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Wed, Mar 09, 2011 at 10:04:11AM -0800, Justin TerAvest wrote:
> >> >> On Tue, Mar 8, 2011 at 2:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > On Tue, Mar 08, 2011 at 05:43:25PM -0500, Vivek Goyal wrote:
> >> >> >> On Tue, Mar 08, 2011 at 01:20:50PM -0800, Justin TerAvest wrote:
> >> >> >> > This patchset adds tracking to the page_cgroup structure for which cgroup has
> >> >> >> > dirtied a page, and uses that information to provide isolation between
> >> >> >> > cgroups performing writeback.
> >> >> >> >
> >> >> >>
> >> >> >> Justin,
> >> >> >>
> >> >> >> So if somebody is trying to isolate a workload which does bunch of READS
> >> >> >> and lots of buffered WRITES, this patchset should help in the sense that
> >> >> >> all the heavy WRITES can be put into a separate cgroup of low weight?
> >> >> >>
> >> >> >> Other application which are primarily doing READS, direct WRITES or little
> >> >> >> bit of buffered WRITES should still get good latencies if heavy writer
> >> >> >> is isolated in a separate group?
> >> >> >>
> >> >> >> If yes, then this piece standalone can make sense. And once the other
> >> >> >> piece/patches of memory cgroup dirty ratio and cgroup aware buffered
> >> >> >> writeout come in, then one will be able to differentiate buffered writes
> >> >> >> of different groups.
> >> >> >
> >> >> > Thinking more about it, currently anyway SYNC preempts the ASYNC. So the
> >> >> > question would be will it help me enable get better isolation latencies
> >> >> > of READS agains buffered WRITES?
> >> >>
> >> >> Ah! Sorry, I left out a patch that disables cross-group preemption.
> >> >> I'll add that to the patchset and email out v2 soon.
> >> >
> >> > Well, what I was referring to that even in current code sync preempts
> >> > all async in CFQ. So it looks like this patchset will not help get
> >> > better latencies in presence of WRITES?
> >>
> >> Hi Vivek,
> >>
> >> I should have been more clear. I forgot to include a patch that
> >> changes the behavior of that preemption. I haven't mailed out v2 yet
> >> because I was also writing a change to put the css_id in pc->flags
> >> instead of its own field.
> >>
> >> The preemption change would look like:
> >>
> >>     Previously, a sync queue in can preempt an async queue in another
> >>     cgroup. This changes that behavior to disallow such preemption.
> >>
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index ab7a216..0494c0c 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -3390,6 +3390,9 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
> >>         if (!cfqq)
> >>                 return false;
> >>
> >> +       if (new_cfqq->cfqg != cfqq->cfqg)
> >> +               return false;
> >> +
> >>         if (cfq_class_idle(new_cfqq))
> >>                 return false;
> >>
> >> @@ -3409,9 +3412,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
> >>         if (rq_is_sync(rq) && !cfq_cfqq_sync(cfqq))
> >>                 return true;
> >>
> >> -       if (new_cfqq->cfqg != cfqq->cfqg)
> >> -               return false;
> >>
> >> I will include the test results that show that isolation is also
> >> improved between a reader and a buffered writer.
> >>
> >
> > Justin,
> >
> > Right now all the async queues go in one cgroup and that is root cgroup.
> > So it is perfectly fine to let sync preempt async.
> >
> > I will be interested in seeing the results. But do you have a theoritical
> > explanation also that why with this patch set we will get better isolation
> > between READS and WRITES?
> 
> Hi Vivek,
> 
> Some of the change is probably because the patch set also changes
> preemption semantics. Because now a group will be charged for the
> buffered WRITES it is causing, it will give less time than before to a
> task that is doing lots of buffered writes. I feel like I am still not
> understanding something you are saying, but it will probably make more
> sense once I have supporting data.
> 
> >
> > Especially after following patch from Shaohua Li.
> >
> > commit f8ae6e3eb8251be32c6e913393d9f8d9e0609489
> > Author: Shaohua Li <shaohua.li@intel.com>
> > Date:   Fri Jan 14 08:41:02 2011 +0100
> >
> >    block cfq: make queue preempt work for queues from different workload
> >
> >
> > This patch will make sure that sync always preempt async. So I am not
> > able to understand that how this patch series provides better latencies
> > for READS in presence of WRITES.
> 
> I have primarily been measuring isolation between READS and WRITES. Do
> you have any tools you'd recommend for getting data on when latencies
> are better?

I like fio. Just launch bunch of random readers and start few writebacks
and notice max completion latency of reads. We can compare the results
with Jen's tree vs your changes on top.

Keep all READS and WRITES in root group (without your patches). With your
patches move WRITES in a separate cgroup and see if you can get better
latencies for READS. So this is WRITES being root group vs WRITES being
in a separate cgroup of low weight.

Thanks
Vivek