[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120323124507.GI13920@gmail.com>
Date: Fri, 23 Mar 2012 13:45:07 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Ananth N Mavinakayanahalli <ananth@...ibm.com>,
Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
LKML <linux-kernel@...r.kernel.org>,
Linux-mm <linux-mm@...ck.org>, Oleg Nesterov <oleg@...hat.com>,
Andi Kleen <andi@...stfloor.org>,
Christoph Hellwig <hch@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
Arnaldo Carvalho de Melo <acme@...radead.org>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] uprobes/core: counter to optimize probe hits.
* Srikar Dronamraju <srikar@...ux.vnet.ibm.com> wrote:
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 9ade86e..62d5aeb 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -390,6 +390,7 @@ struct mm_struct {
> struct cpumask cpumask_allocation;
> #endif
> #ifdef CONFIG_UPROBES
> + atomic_t mm_uprobes_count;
> struct uprobes_xol_area *uprobes_xol_area;
> #endif
Since mm_types.h includes uprobes.h already it's much better to
stick this into a 'struct uprobes_state' and thus keep the main
'struct mm_struct' definition as simple as possible.
Also, your patch titles suck:
- no proper capitalization like you can observe with previous
uprobes commits
- extra period at the end
- missing verb from the sentence. Check existing uprobes
commits to see the kind of sentences that commit titles are
expected to be.
> + if (!atomic_read(&uprobe_events) || !valid_vma(vma, false))
> + return; /* Bail-out */
> +
> + if (!atomic_read(&vma->vm_mm->mm_uprobes_count))
> + return;
> +
> + inode = vma->vm_file->f_mapping->host;
> + if (!inode)
> + return;
The 'Bail-out' comment tacked on to one of the returns seems
entirely superfluous.
> + if (vaddr >= vma->vm_start && vaddr < vma->vm_end) {
> +
> + /*
That newline looks superfluous too.
> + return;
> +}
Please read your own patches more carefully ... this should
stick out like a sore thumb.
Some of the above comments apply to your other patch as well.
The structure and granularity of the patches looks good
otherwise.
Thanks,
Ingo
--
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/
Powered by blists - more mailing lists