Message-ID: <20100921082007.GA12118@haskell.muteddisk.com>
Date:	Tue, 21 Sep 2010 01:20:07 -0700
From:	matt mooney <mfm@...eddisk.com>
To:	Jason Baron <jbaron@...hat.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu,
	mathieu.desnoyers@...ymtl.ca, hpa@...or.com, tglx@...utronix.de,
	rostedt@...dmis.org, andi@...stfloor.org, roland@...hat.com,
	rth@...hat.com, mhiramat@...hat.com, fweisbec@...il.com,
	avi@...hat.com, davem@...emloft.net, vgoyal@...hat.com,
	sam@...nborg.org, tony@...eyournoodle.com
Subject: Re: [PATCH 10/10] jump label v11: add docs

On 11:09 Fri 17 Sep     , Jason Baron wrote:
> Add jump label docs as: Documentation/jump-label.txt
> 
> +Currently, tracepoints are implemented using a conditional. The conditional
> +check requires checking a global variable for each tracepoint. Although,

No comma after although here.

> +the overhead of this check is small, it increases under memory pressure. As we
> +increase the number of tracepoints in the kernel this may become more of an

Comma after kernel is needed to separate the preposition, and I think the use of
"this" in the sentence is unclear.

> +issue. In addition, tracepoints are often dormant (disabled), and provide no

Also, no comma before "and" because the second part is a subordinate clause.

> +direct kernel functionality. Thus, it is highly desirable to reduce their
> +impact as much as possible. Although tracepoints are the original motivation
> +for this work, other kernel code paths should be able to make use of the jump
> +label optimization.
> +
> +
> +For architectures that have not yet introduced jump label support its simply:

..."support, it's" ...

> +
> +Thus, when tracing is disabled, we simply have a no-op followed by a jump around
> +the dormant (disabled) tracing code. The 'JUMP_LABEL()' macro, produces a
> +'jump_table' which has the following format:

No comma after macro, but a comma is needed after jump_table because what
follows is a nonrestrictive clause.

> +[instruction address] [jump target] [tracepoint key]
> +
> +Thus, to enable a tracepoint, we simply patch the 'instruction address' with
> +a jump to the 'jump target'.

Punctuation is suppose to go inside quotes (I know it's ugly and illogical).

> +
> +The call to enable a jump label is: enable_jump_label(key); to disable:
> +disable_jump_label(key);

Hmm, maybe a better structure would be:
"The calls to enable and disable a jump label are: enable_jump_label(key) and
disable_jump_label(key)."

> +There are a few functions and macros which arches must implement in order to

"That" should be used here because it is restrictive.

> +take advantage of this optimization. As previously mentioned, if there is no
> +architecture support we simply fall back to a traditional, load, test, and

Comma after support.

> +
> +In terms of code analysis the current code for the disabled case is a 'cmpl'

Comma after analysis; it is a preposition.

> +followed by a 'je' around the tracepoint code. so:

Capitalize S in "so"

> +
> +
> +The optimization depends on !CC_OPTIMIZE_FOR_SIZE. When CC_OPTIMIZE_FOR_SIZE is
> +set, gcc does not always out of line the not taken label path in the same way
> +that the "if unlikely()" paths are made out of line. Thus, with
> +CC_OPTIMIZE_FOR_SIZE set, this optimization is not always optimal. This may be
> +solved in subsequent gcc versions, that allow us to move labels out of line,

"which" should be used here instead of that.

Otherwise, from a writing standpoint, it looks good.

-mfm

--
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