Re: [PATCH] ext4/jbd2: remove stray markers

Re: [PATCH] ext4/jbd2: remove stray markers

[Ingo Molnar]

* Christoph Hellwig <hch@lst.de> wrote:

> ext4/jbd2 has a couple of stray markers without any users 
> introduced in commit ba80b1019aa722b24506db1ee755e0bb2f513022 
> (which has a very useless changelog, btw).  Remove them in 
> preparation of removing the markers in favour of the TRACE_EVENT 
> macro (and also because we don't keep dead code around).
> 
> Ted, I think you have some TRAVE_EVENT patches for ext4 pending, 
> but is it okay to queue up this removal in the tracing tree?  That 
> way we can remove the markers leftovers there completely as soon 
> as the 2.6.31 merge window opens.

i think these markers are still in active use, so i'd not remove 
them before Ted's TRACE_EVENT() changes are included. We can/should 
do that in a single topic - in a work flow that suits Ted best.

We can do a -git based special-purpose topic branch in -tip, or we 
can do it in tip/tracing, or we can pull a (-git based) branch from 
Ted. Or we can delay it all to the v2.6.31 merge window. Ted's 
choice.

	Ingo

Re: [PATCH] ext4/jbd2: remove stray markers

[Christoph Hellwig]

On Wed, May 06, 2009 at 01:03:54PM +0200, Ingo Molnar wrote:
> i think these markers are still in active use,

It's trivially proveable by grep that there is real no user.
(modulo ad-hoc out of tree modules, which we've never cared about
retaining stuff)

The workflow question is the real one, that's why I can only do it if
Ted is okay with it.

Re: [PATCH] ext4/jbd2: remove stray markers

[Ingo Molnar]

* Christoph Hellwig <hch@lst.de> wrote:

> On Wed, May 06, 2009 at 01:03:54PM +0200, Ingo Molnar wrote:
> > i think these markers are still in active use,
> 
> It's trivially proveable by grep that there is real no user.
> (modulo ad-hoc out of tree modules, which we've never cared about
> retaining stuff)

Well, i'll defer to Ted about whether those out of tree uses are 
relevant to ext4. It's not a problem to do this all in a polite way, 
without destroying functionality.

	Ingo

Re: [PATCH] ext4/jbd2: remove stray markers

[Theodore Tso]

On Wed, May 06, 2009 at 01:03:54PM +0200, Ingo Molnar wrote:
> 
> > Ted, I think you have some TRAVE_EVENT patches for ext4 pending, 
> > but is it okay to queue up this removal in the tracing tree?  That 
> > way we can remove the markers leftovers there completely as soon 
> > as the 2.6.31 merge window opens.
> 
> i think these markers are still in active use, so i'd not remove 
> them before Ted's TRACE_EVENT() changes are included. We can/should 
> do that in a single topic - in a work flow that suits Ted best.

My complaint with Cristoph's is that it will conflict with patches I
have pending which replaces the markers with tracepoints --- and I
*am* using the tracepoints actively.  The only reason why these
patches aren't in -stable is they have a dependency one of Rostedt's
changes.  (Not a syntactic dependency, but if we merge in the wrong
order, and the rcu_read_lock/unlock() calls aren't around the
TP_PRINTK callpoint, then in certain CONFIG_PREEMPT configurations and
if there is more than one active ext4 filesystem while the ext4 or
jbd2 tracepoints are active, there's a potential race.)

> We can do a -git based special-purpose topic branch in -tip, or we 
> can do it in tip/tracing, or we can pull a (-git based) branch from 
> Ted. Or we can delay it all to the v2.6.31 merge window. Ted's 
> choice.

My plan was to wait for the tracing patches to get pushed during the
2.6.31 merge tree, at which point I would then push my changes which
replace the markers with TRACE_EVENT changes.  So no matter which way
we do this, the ext4 markers will be gone by the end of the 2.6.31
merge window.

Christoph, if you have some desire to completely remove the
CONFIG_MARKERS support code, and I'm holding up your ability to do
work, I can take the ext4 TRACE_EVENT patches, and queue them up in
tip/tracing.  It's less work than if we take your markers removal
patches, since then I would have to resolve all of the conflicts with
my patches which replace all of the ext4 and jbd2 markers with
TRACE_EVENTS macros.

My preference is for the former, mainly because my patches are already
set up for that, and I'm a lazy bastard; the latter wouldn't be much
work, though.  I'm guessing your preference would be for the latter?

	      		       	      - Ted

Re: [PATCH] ext4/jbd2: remove stray markers

[Christoph Hellwig]

On Wed, May 06, 2009 at 07:40:51AM -0400, Theodore Tso wrote:
> My complaint with Cristoph's

Christoph, still :)

> is that it will conflict with patches I
> have pending which replaces the markers with tracepoints --- and I
> *am* using the tracepoints actively.  The only reason why these
> patches aren't in -stable is they have a dependency one of Rostedt's
> changes.  (Not a syntactic dependency, but if we merge in the wrong
> order, and the rcu_read_lock/unlock() calls aren't around the
> TP_PRINTK callpoint, then in certain CONFIG_PREEMPT configurations and
> if there is more than one active ext4 filesystem while the ext4 or
> jbd2 tracepoints are active, there's a potential race.)

Also without Steve's updates you can't actually use them in a module.

> My plan was to wait for the tracing patches to get pushed during the
> 2.6.31 merge tree, at which point I would then push my changes which
> replace the markers with TRACE_EVENT changes.  So no matter which way
> we do this, the ext4 markers will be gone by the end of the 2.6.31
> merge window.
> 
> Christoph, if you have some desire to completely remove the
> CONFIG_MARKERS support code, and I'm holding up your ability to do
> work, I can take the ext4 TRACE_EVENT patches, and queue them up in
> tip/tracing.  It's less work than if we take your markers removal
> patches, since then I would have to resolve all of the conflicts with
> my patches which replace all of the ext4 and jbd2 markers with
> TRACE_EVENTS macros.

I was planning to do make sure it's all gone.  Once we are into the
2.6.31 merge dinwo I fear that the final patches will miss the window
due to all these interdependencies.  But Ingo didn't seem to be too
interested in taking the other patches that would be required for it,
so I guess I'll try to somehow get it done in the merge window and
otherwise we'll have to wait for 2.6.32.

Re: [PATCH] ext4/jbd2: remove stray markers

[Ingo Molnar]

* Christoph Hellwig <hch@lst.de> wrote:

> I was planning to do make sure it's all gone.  Once we are into 
> the 2.6.31 merge dinwo I fear that the final patches will miss the 
> window due to all these interdependencies.  But Ingo didn't seem 
> to be too interested in taking the other patches that would be 
> required for it, so I guess I'll try to somehow get it done in the 
> merge window and otherwise we'll have to wait for 2.6.32.

Well, while coupling to lots of subsystems is natural for something 
as intrinsic as the tracing tree - still i dont want to over-do it. 
In little over a month these patches can go into their local 
subsystem trees, without any interactions with anything.

So the patches you did are nice - just (as it is usual with any core 
kernel tree) the logistics have to be planned carefully.

	Ingo

Re: [PATCH] ext4/jbd2: remove stray markers

[Theodore Tso]

On Wed, May 06, 2009 at 01:44:15PM +0200, Christoph Hellwig wrote:
> On Wed, May 06, 2009 at 07:40:51AM -0400, Theodore Tso wrote:
> > My complaint with Cristoph's
> 
> Christoph, still :)

Sorry, I keep making that mistake!  Keep calling me on it, and
eventually I'll reprogram those finger macros.  :-)


> Also without Steve's updates you can't actually use them in a module.

Yep; not sure *how* critical that would be for -next, though.
Probably there would be some complaints....

In any case, it'll be less work to worry about patch conflicts if we
just do it during the merge window.  If we can just arrange to push
the tip/tracing changes early, I'll push the marker replacement
patches as soon as I see them hit mainline.

Regards,

						- Ted

Re: [PATCH] ext4/jbd2: remove stray markers

[Ingo Molnar]

* Theodore Tso <tytso@mit.edu> wrote:

> On Wed, May 06, 2009 at 01:44:15PM +0200, Christoph Hellwig wrote:
> > On Wed, May 06, 2009 at 07:40:51AM -0400, Theodore Tso wrote:
> > > My complaint with Cristoph's
> > 
> > Christoph, still :)
> 
> Sorry, I keep making that mistake!  Keep calling me on it, and
> eventually I'll reprogram those finger macros.  :-)
> 
> 
> > Also without Steve's updates you can't actually use them in a module.
> 
> Yep; not sure *how* critical that would be for -next, though. 
> Probably there would be some complaints....
> 
> In any case, it'll be less work to worry about patch conflicts if 
> we just do it during the merge window.  If we can just arrange to 
> push the tip/tracing changes early, I'll push the marker 
> replacement patches as soon as I see them hit mainline.

Cool, thanks! I'll try to aim for an early merge of the tracing 
tree. (And such dependent patches are generally possible shortly 
after -rc1 as well, especially if the clean-up factor is 
significant.)

	Ingo

Re: [PATCH] ext4/jbd2: remove stray markers

[Ingo Molnar]

* Theodore Tso <tytso@mit.edu> wrote:

> On Wed, May 06, 2009 at 01:03:54PM +0200, Ingo Molnar wrote:
> > 
> > > Ted, I think you have some TRAVE_EVENT patches for ext4 pending, 
> > > but is it okay to queue up this removal in the tracing tree?  That 
> > > way we can remove the markers leftovers there completely as soon 
> > > as the 2.6.31 merge window opens.
> > 
> > i think these markers are still in active use, so i'd not remove 
> > them before Ted's TRACE_EVENT() changes are included. We can/should 
> > do that in a single topic - in a work flow that suits Ted best.
> 
> My complaint with Cristoph's is that it will conflict with patches 
> I have pending which replaces the markers with tracepoints --- and 
> I *am* using the tracepoints actively.  The only reason why these 
> patches aren't in -stable is they have a dependency one of 
> Rostedt's changes.  (Not a syntactic dependency, but if we merge 
> in the wrong order, and the rcu_read_lock/unlock() calls aren't 
> around the TP_PRINTK callpoint, then in certain CONFIG_PREEMPT 
> configurations and if there is more than one active ext4 
> filesystem while the ext4 or jbd2 tracepoints are active, there's 
> a potential race.)

That's OK.

> > We can do a -git based special-purpose topic branch in -tip, or 
> > we can do it in tip/tracing, or we can pull a (-git based) 
> > branch from Ted. Or we can delay it all to the v2.6.31 merge 
> > window. Ted's choice.
> 
> My plan was to wait for the tracing patches to get pushed during 
> the 2.6.31 merge tree, at which point I would then push my changes 
> which replace the markers with TRACE_EVENT changes.  So no matter 
> which way we do this, the ext4 markers will be gone by the end of 
> the 2.6.31 merge window.

That's a perfectly fine approach.

> Christoph, if you have some desire to completely remove the 
> CONFIG_MARKERS support code, and I'm holding up your ability to do 
> work, I can take the ext4 TRACE_EVENT patches, and queue them up 
> in tip/tracing.  It's less work than if we take your markers 
> removal patches, since then I would have to resolve all of the 
> conflicts with my patches which replace all of the ext4 and jbd2 
> markers with TRACE_EVENTS macros.
> 
> My preference is for the former, mainly because my patches are 
> already set up for that, and I'm a lazy bastard; the latter 
> wouldn't be much work, though.  I'm guessing your preference would 
> be for the latter?

No, lets delay this to the v2.6.31 merge window. I'd like to remove 
markers - but not at the cost of making life harder for others and 
at the cost of creating inter-dependencies on such a level.

I objected to markers back when they were merged in a rather 
haphazard way, but i'll object to any haphazard removal just as much 
;-) The .31 merge window will open in about a month.

	Ingo