Re: [PATCH 4/5] trace: Make removal of ring buffer pages atomic

[David Sharp <dhsharp@google.com>]

On Fri, Jul 29, 2011 at 7:43 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-07-29 at 18:50 -0700, David Sharp wrote:
>> On Fri, Jul 29, 2011 at 6:12 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Fri, 2011-07-29 at 16:30 -0700, Vaibhav Nagarnaik wrote:

>> What is it that you don't like? the work/completion, the reliance on
>> running on the same cpu, or just the complexity of procedure?
>
> The added complexity. This is complex enough, we don't need to make it
> more so.

Sure, complexity should be part of the cost-benefit analysis. I think
this will be a pretty powerful feature, though. Let's see how it goes;
maybe Vaibhav and I can come up with something simpler, or using
established protocols.

>> > Anyway, the choices are:
>> >
>> > * Remove from the HEAD and use the existing algorithm that we've been
>> > using since 2008. This requires a bit of accounting on the reader side,
>> > but nothing too complex.
>> >
>> > Pros: Should not have any major race conditions. Requires no
>> > schedule_work_on() calls. Uses existing algorithm
>> >
>> > Cons: Can keep pages around if no reader is present, and ring buffer is
>> > not reset.
>>
>> Con: by definition, removes valid trace data from the ring buffer,
>> even if it is not full. I think that's a pretty big con for the
>> usability of the feature.
>
> Um, how does it remove valid trace data? We don't free it, we off load
> it. Think of it as "extended reader pages". That is, they are held off
> until the user asks to read these pages. Then they will get the data
> again. What is a con about that?

I think we're talking about different things. You're talking about
keeping the "removed" pages around for the reader if it wants it,
whereas I'm talking about trying to free the pages. In our use case,
we're trying to free up memory, so we would want to immediately use
the "flush the extended reader pages" control file you suggested
below. So, in effect, for us it really is the same as removing valid
trace data, even if there are empty pages. "Offloading" the pages
isn't really good enough. It's another interesting use case, but
doesn't meet the goal of this patch series.

Maybe the use case hasn't been stated coherently: We're in overwrite
mode (but perhaps still before overflow has happened), not reading the
trace yet, waiting for something interesting to happen. In the
meantime, CPUs have varying rates of events occurring on them, and we
have only so much memory on the system set aside for tracing. In order
to efficiently use that memory, we want to adjust the sizes of the
per-cpu buffers in flight so that each CPU has approximately the same
time span, and for as far back as possible within our memory
allocation. Therefore, we want to free empty pages first, and then the
pages with the oldest data.

>
>>
>> >
>> > * Read from tail. Modify the already complex but tried and true lockless
>> > algorithm.
>> >
>> > Pros: Removes empty pages first.
>> >
>> > Cons: Adds a lot more complexity to a complex system that has been
>> > working since 2008.
>> >
>> >
>> > The above makes me lean towards just taking from HEAD.
>> >
>> > If you are worried about leaked pages, we could even have a debugfs file
>> > that lets us monitor the pages that are pending read, and have the user
>> > (or application) be able to flush them if they see the ring buffer is
>> > full anyway.
>>
>> The reason we want per-cpu dynamic resizing is to increase memory
>> utilization, so leaking pages would make me sad.
>
> Shouldn't be too leaky, especially if something can read it. Perhaps we
> could figure out a way to swap them back in.
>
>>
>> Let us mull it over this weekend... maybe we'll come up with something
>> that works more simply.
>
> Hmm, actually, we could take an idea that Mathieu used for his ring
> buffer. He couldn't swap out a page if the writer was on it, so he would
> send out ipi's to push the writer off the page and just pad the rest.

hmm, I'm not seeing how we could use that technique without dropping
recent events. Dropping older events is preferable, in which case we
might as well remove from the head page. I'll add it to my toolbox
though, as I think about it.

> (if it doesn't work with interrupts enabled, it wont
> work for NMIs, so I will not accept disabling interrupts)

I agree, we should not disable interrupts.