linux-kernel - Re: [PATCH v3] trace: Add per_cpu ring buffer control files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 6 Sep 2011 11:56:30 -0700
From:	Vaibhav Nagarnaik <vnagarnaik@...gle.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Michael Rubin <mrubin@...gle.com>,
	David Sharp <dhsharp@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] trace: Add per_cpu ring buffer control files

On Fri, Sep 2, 2011 at 7:45 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> On Mon, 2011-08-22 at 18:17 -0700, Vaibhav Nagarnaik wrote:
>> +     /* ring buffer pages to update, > 0 to add, < 0 to remove */
>> +     int                             nr_pages_to_update;
>> +     struct list_head                new_pages; /* new pages to add */
>
> There's no reason for the added new_pages. And I'm not sure I even like
> the 'nr_pages_to_update' either. These are only used for resizing and
> are just wasting space otherwise.
>
> You could allocate an array of numbers for the nr_pages_to_update and
> use that instead. As for the list, heck, you can still use a single list
> and pass that around like the original code did.
>

In this patch's context, I could still use the same logic of having a
temporary list of new pages and passing the nr_pages_to_update as a
parameter to the function. However, this comes in handy when considering
the next 2 patches. The updates are meant to be per_cpu and so having
separate list of new pages is better to handle. Same thing with
nr_pages_to_update.

>> +                             update_pages_handler(cpu_buffer);
>> +                             cpu_buffer->nr_pages = nr_pages;
>
> The two places that call update_pages_handler() also updates
> cpu_buffer->nr_pages. Move that to the update_pages_handler() as well.

Sure. I will update that.

> I'm still very nervous about this patch. I'm going to hold off a release
> cycle before even giving it up to Ingo.

Sure. I agree that the patches need a rigorous review and your
confidence in them is a necessary element.

I am working on other projects now, so I might have a slower response
time. But I will try to update the patch and send it to you in a timely
manner.


Vaibhav Nagarnaik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/