[PATCH] replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c

[PATCH] replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c

[Steven Rostedt]

The following patch is in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: cleanups


Steven Rostedt (1):
      replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c

----
 net/dccp/proto.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit 3e1de819261b68a48b09719694c7e3579a8e4186
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Dec 10 12:49:47 2008 -0500

    replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c
    
    Impact: clean up
    
    RW_LOCK_UNLOCKED has been deprecated and superseded by
    __RW_LOCK_UNLOCKED(name).  This patch removes one of the last
    remaining users of it.
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index d0bd348..e6e54a7 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -45,7 +45,7 @@ atomic_t dccp_orphan_count = ATOMIC_INIT(0);
 EXPORT_SYMBOL_GPL(dccp_orphan_count);
 
 struct inet_hashinfo __cacheline_aligned dccp_hashinfo = {
-	.lhash_lock	= RW_LOCK_UNLOCKED,
+	.lhash_lock	= __RW_LOCK_UNLOCKED(dccp_hashinfo.lhash_lock),
 	.lhash_users	= ATOMIC_INIT(0),
 	.lhash_wait = __WAIT_QUEUE_HEAD_INITIALIZER(dccp_hashinfo.lhash_wait),
 };

Re: [PATCH] replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c

[Eric Dumazet]

cced netdev

Steven Rostedt a écrit :
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: cleanups
> 
> 
> Steven Rostedt (1):
>       replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c
> 
> ----
>  net/dccp/proto.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> ---------------------------
> commit 3e1de819261b68a48b09719694c7e3579a8e4186
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Wed Dec 10 12:49:47 2008 -0500
> 
>     replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c
>     
>     Impact: clean up
>     
>     RW_LOCK_UNLOCKED has been deprecated and superseded by
>     __RW_LOCK_UNLOCKED(name).  This patch removes one of the last
>     remaining users of it.
>     
>     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> diff --git a/net/dccp/proto.c b/net/dccp/proto.c
> index d0bd348..e6e54a7 100644
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -45,7 +45,7 @@ atomic_t dccp_orphan_count = ATOMIC_INIT(0);
>  EXPORT_SYMBOL_GPL(dccp_orphan_count);
>  
>  struct inet_hashinfo __cacheline_aligned dccp_hashinfo = {
> -	.lhash_lock	= RW_LOCK_UNLOCKED,
> +	.lhash_lock	= __RW_LOCK_UNLOCKED(dccp_hashinfo.lhash_lock),
>  	.lhash_users	= ATOMIC_INIT(0),
>  	.lhash_wait = __WAIT_QUEUE_HEAD_INITIALIZER(dccp_hashinfo.lhash_wait),
>  };
> --

Hum... this rwlock doesnt exist anymore on net-next-2.6, all this stuff
was converted to RCU for upcoming 2.6.29

struct inet_hashinfo dccp_hashinfo;
EXPORT_SYMBOL_GPL(dccp_hashinfo);

I guess such a patch wont ease David job when 2.6.29 merge window opens

Re: [PATCH] replace deprecated RW_LOCK_UNLOCKED in net/dccp/proto.c

[Steven Rostedt]

On Wed, 10 Dec 2008, Eric Dumazet wrote:

> cced netdev

Thanks,

> 
> Steven Rostedt a ?crit :
> >  
> >  struct inet_hashinfo __cacheline_aligned dccp_hashinfo = {
> > -	.lhash_lock	= RW_LOCK_UNLOCKED,
> > +	.lhash_lock	= __RW_LOCK_UNLOCKED(dccp_hashinfo.lhash_lock),
> >  	.lhash_users	= ATOMIC_INIT(0),
> >  	.lhash_wait = __WAIT_QUEUE_HEAD_INITIALIZER(dccp_hashinfo.lhash_wait),
> >  };
> > --
> 
> Hum... this rwlock doesnt exist anymore on net-next-2.6, all this stuff
> was converted to RCU for upcoming 2.6.29
> 
> struct inet_hashinfo dccp_hashinfo;
> EXPORT_SYMBOL_GPL(dccp_hashinfo);
> 
> I guess such a patch wont ease David job when 2.6.29 merge window opens

One of the requirements in the new rt git tree is that all rwlocks must 
use the __RW_LOCK_UNLOCK(lock) macro. I had to fix it in our tree, but 
whenever I do a clean up patch, I like to share it ;-)

-- Steve

[PATCH] update rwlock initialization for nat_table

[Steven Rostedt]

The following patch is in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: cleanups


Steven Rostedt (1):
      update rwlock initialization for nat_table

----
 net/ipv4/netfilter/nf_nat_rule.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit d4175059c8f95e4cd58e0efaa85610ca59469fbd
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Dec 10 15:00:09 2008 -0500

    update rwlock initialization for nat_table
    
    Impact: clean up
    
    The commit e099a173573ce1ba171092aee7bb3c72ea686e59
    (netfilter: netns nat: per-netns NAT table) renamed the
    nat_table from __nat_table to nat_table without updating the
    __RW_LOCK_UNLOCKED(__nat_table.lock).
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/net/ipv4/netfilter/nf_nat_rule.c b/net/ipv4/netfilter/nf_nat_rule.c
index bea54a6..8d489e7 100644
--- a/net/ipv4/netfilter/nf_nat_rule.c
+++ b/net/ipv4/netfilter/nf_nat_rule.c
@@ -61,7 +61,7 @@ static struct
 static struct xt_table nat_table = {
 	.name		= "nat",
 	.valid_hooks	= NAT_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(__nat_table.lock),
+	.lock		= __RW_LOCK_UNLOCKED(nat_table.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET,
 };

Re: [PATCH] update rwlock initialization for nat_table

[Andrew Morton]

On Wed, 10 Dec 2008 15:06:00 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: cleanups
> 
> 
> Steven Rostedt (1):
>       update rwlock initialization for nat_table
> 
> ----
>  net/ipv4/netfilter/nf_nat_rule.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> ---------------------------
> commit d4175059c8f95e4cd58e0efaa85610ca59469fbd
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Wed Dec 10 15:00:09 2008 -0500
> 
>     update rwlock initialization for nat_table
>     
>     Impact: clean up

It's more than a "cleanup"?

>     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
>     (netfilter: netns nat: per-netns NAT table) renamed the
>     nat_table from __nat_table to nat_table without updating the
>     __RW_LOCK_UNLOCKED(__nat_table.lock).
>     
>     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> diff --git a/net/ipv4/netfilter/nf_nat_rule.c b/net/ipv4/netfilter/nf_nat_rule.c
> index bea54a6..8d489e7 100644
> --- a/net/ipv4/netfilter/nf_nat_rule.c
> +++ b/net/ipv4/netfilter/nf_nat_rule.c
> @@ -61,7 +61,7 @@ static struct
>  static struct xt_table nat_table = {
>  	.name		= "nat",
>  	.valid_hooks	= NAT_VALID_HOOKS,
> -	.lock		= __RW_LOCK_UNLOCKED(__nat_table.lock),
> +	.lock		= __RW_LOCK_UNLOCKED(nat_table.lock),
>  	.me		= THIS_MODULE,
>  	.af		= AF_INET,
>  };

At present any lockdep messages relating to this lock will print the
wrong name.  So it's a nanobug, I think?

Re: [PATCH] update rwlock initialization for nat_table

[Steven Rostedt]

On Thu, 11 Dec 2008, Andrew Morton wrote:

> On Wed, 10 Dec 2008 15:06:00 -0500 (EST)
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > ---------------------------
> > commit d4175059c8f95e4cd58e0efaa85610ca59469fbd
> > Author: Steven Rostedt <srostedt@redhat.com>
> > Date:   Wed Dec 10 15:00:09 2008 -0500
> > 
> >     update rwlock initialization for nat_table
> >     
> >     Impact: clean up
> 
> It's more than a "cleanup"?
> 
> >     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
> >     (netfilter: netns nat: per-netns NAT table) renamed the
> >     nat_table from __nat_table to nat_table without updating the
> >     __RW_LOCK_UNLOCKED(__nat_table.lock).
> >     
> >     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > 
> > diff --git a/net/ipv4/netfilter/nf_nat_rule.c b/net/ipv4/netfilter/nf_nat_rule.c
> > index bea54a6..8d489e7 100644
> > --- a/net/ipv4/netfilter/nf_nat_rule.c
> > +++ b/net/ipv4/netfilter/nf_nat_rule.c
> > @@ -61,7 +61,7 @@ static struct
> >  static struct xt_table nat_table = {
> >  	.name		= "nat",
> >  	.valid_hooks	= NAT_VALID_HOOKS,
> > -	.lock		= __RW_LOCK_UNLOCKED(__nat_table.lock),
> > +	.lock		= __RW_LOCK_UNLOCKED(nat_table.lock),
> >  	.me		= THIS_MODULE,
> >  	.af		= AF_INET,
> >  };
> 
> At present any lockdep messages relating to this lock will print the
> wrong name.  So it's a nanobug, I think?

Well, I'm now working on the RT git tree, and it has a much stronger 
requirement on __RW_LOCK_UNLOCK. It actually uses the variable inside.

For mainline, it is a nanobug, but for -rt it is a compiler error. Since 
it is still a clean up, it would be nice to have it in mainline ;-)

-- Steve

Re: [PATCH] update rwlock initialization for nat_table

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Wed, 10 Dec 2008 15:06:00 -0500 (EST)

>     update rwlock initialization for nat_table
>     
>     Impact: clean up
>     
>     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
>     (netfilter: netns nat: per-netns NAT table) renamed the
>     nat_table from __nat_table to nat_table without updating the
>     __RW_LOCK_UNLOCKED(__nat_table.lock).
>     
>     Signed-off-by: Steven Rostedt <srostedt@redhat.com>

Applied to net-2.6, thanks Steven.

As Andrew mentioned this is a bug (albeit a "nano-bug" as you
called it :-) so I removed the Impact line in the commit
message when applying this.

Thanks!

Impact: (was Re: [PATCH] update rwlock initialization for nat_table)

[Alexey Dobriyan]

On Mon, Dec 15, 2008 at 12:20:19AM -0800, David Miller wrote:
> >     update rwlock initialization for nat_table
> >     
> >     Impact: clean up
> >     
> >     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
> >     (netfilter: netns nat: per-netns NAT table) renamed the
> >     nat_table from __nat_table to nat_table without updating the
> >     __RW_LOCK_UNLOCKED(__nat_table.lock).
> >     
> >     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> Applied to net-2.6, thanks Steven.
> 
> As Andrew mentioned this is a bug (albeit a "nano-bug" as you
> called it :-) so I removed the Impact line in the commit
> message when applying this.

Speaking of Impact: lines, is this a new fashion or what?

Looking at the ones which are already in official tree, they are either
trivially duplicating Subject: line, or effectively duplicating Subject: line,
or cover up for insufficiently informative (read: badly written) Subject: line,
or simply useless.


	Subject: sched: CPU remove deadlock fix
	Impact: fix possible deadlock in CPU hot-remove path

What prevented to write "Subject: sched: fix possible deadlock in CPU hot-remove path"?


	AMD IOMMU: __unmap_single: check for bad_dma_address instead of 0
	Impact: minor fix

Well...

I have an idea on how to make them remotely useful, but can we agree that there is
a problem arising here?

Re: Impact: (was Re: [PATCH] update rwlock initialization for nat_table)

[Andrew Morton]

On Tue, 16 Dec 2008 04:10:39 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Dec 15, 2008 at 12:20:19AM -0800, David Miller wrote:
> > >     update rwlock initialization for nat_table
> > >     
> > >     Impact: clean up
> > >     
> > >     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
> > >     (netfilter: netns nat: per-netns NAT table) renamed the
> > >     nat_table from __nat_table to nat_table without updating the
> > >     __RW_LOCK_UNLOCKED(__nat_table.lock).
> > >     
> > >     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > 
> > Applied to net-2.6, thanks Steven.
> > 
> > As Andrew mentioned this is a bug (albeit a "nano-bug" as you
> > called it :-) so I removed the Impact line in the commit
> > message when applying this.
> 
> Speaking of Impact: lines, is this a new fashion or what?
> 
> Looking at the ones which are already in official tree, they are either
> trivially duplicating Subject: line, or effectively duplicating Subject: line,
> or cover up for insufficiently informative (read: badly written) Subject: line,
> or simply useless.
> 
> 
> 	Subject: sched: CPU remove deadlock fix
> 	Impact: fix possible deadlock in CPU hot-remove path
> 
> What prevented to write "Subject: sched: fix possible deadlock in CPU hot-remove path"?
> 
> 
> 	AMD IOMMU: __unmap_single: check for bad_dma_address instead of 0
> 	Impact: minor fix
> 
> Well...
> 
> I have an idea on how to make them remotely useful, but can we agree that there is
> a problem arising here?

heh, I must say that the ones I've seen haven't been very useful.


However...  Given the amount of time I (and others, to a lesser extent)
spend complaining about and scratching heads over crappy changelogs, we
would benefit from having a standard changelog template.

Something which guides people to creating a good changelog.  But it
would have to be short, and carefully written.  It should learn from
history, to wit:

- ./REPORTING-BUGS has a template and afaik it has never elicited any
  useful information.

- Documentation/SubmittingPatches has info on how to write a
  changelog, and people blithely ignore it.

- kerneldoc provide a template of sorts, and we see that filling out
  templates puts people's brains into "filling out a template" mode,
  rather than into "communicating information" mode.

An interesting problem.

Re: Impact: (was Re: [PATCH] update rwlock initialization for nat_table)

[Arnaldo Carvalho de Melo]

Em Mon, Dec 15, 2008 at 05:19:35PM -0800, Andrew Morton escreveu:
> On Tue, 16 Dec 2008 04:10:39 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Speaking of Impact: lines, is this a new fashion or what?
> > 
> > Looking at the ones which are already in official tree, they are either
> > trivially duplicating Subject: line, or effectively duplicating Subject: line,
> > or cover up for insufficiently informative (read: badly written) Subject: line,
> > or simply useless.
> > 
> > 
> > 	Subject: sched: CPU remove deadlock fix
> > 	Impact: fix possible deadlock in CPU hot-remove path
> > 
> > What prevented to write "Subject: sched: fix possible deadlock in CPU hot-remove path"?
> > 
> > 
> > 	AMD IOMMU: __unmap_single: check for bad_dma_address instead of 0
> > 	Impact: minor fix
> > 
> > Well...
> > 
> > I have an idea on how to make them remotely useful, but can we agree that there is
> > a problem arising here?
> 
> heh, I must say that the ones I've seen haven't been very useful.
> 
> However...  Given the amount of time I (and others, to a lesser extent)
> spend complaining about and scratching heads over crappy changelogs, we
> would benefit from having a standard changelog template.
> 
> Something which guides people to creating a good changelog.  But it
> would have to be short, and carefully written.  It should learn from
> history, to wit:
> 
> - ./REPORTING-BUGS has a template and afaik it has never elicited any
>   useful information.
> 
> - Documentation/SubmittingPatches has info on how to write a
>   changelog, and people blithely ignore it.
> 
> - kerneldoc provide a template of sorts, and we see that filling out
>   templates puts people's brains into "filling out a template" mode,
>   rather than into "communicating information" mode.
> 
> An interesting problem.

Well, if git commit could add that template in addition to the
Signed-off-by line, that could be a start, perhaps as a new option and
then it would get it from .git/changelog-template, that would be
provided by each project.

/me scratches head...

    -t, --template <FILE>
                              use specified template file

Its there already, but then perhaps what is needed is a _default_
template.

- Arnaldo

Re: Impact:

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Tue, 16 Dec 2008 04:10:39 +0300

> On Mon, Dec 15, 2008 at 12:20:19AM -0800, David Miller wrote:
> > >     update rwlock initialization for nat_table
> > >     
> > >     Impact: clean up
> > >     
> > >     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
> > >     (netfilter: netns nat: per-netns NAT table) renamed the
> > >     nat_table from __nat_table to nat_table without updating the
> > >     __RW_LOCK_UNLOCKED(__nat_table.lock).
> > >     
> > >     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > 
> > Applied to net-2.6, thanks Steven.
> > 
> > As Andrew mentioned this is a bug (albeit a "nano-bug" as you
> > called it :-) so I removed the Impact line in the commit
> > message when applying this.
> 
> Speaking of Impact: lines, is this a new fashion or what?

I don't know what it is, but I find them pointless.

Just provide a proper verbose commit message and be done with it.

At the rate we're going we'll soon have commit messages like:

--------------------
Impact: Crashes kernel
Cause: Race between timer and workqueue
Implementation: ...
--------------------

I mean, spare me...

Re: Impact: (was Re: [PATCH] update rwlock initialization for nat_table)

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Dec 15, 2008 at 12:20:19AM -0800, David Miller wrote:
> > >     update rwlock initialization for nat_table
> > >     
> > >     Impact: clean up
> > >     
> > >     The commit e099a173573ce1ba171092aee7bb3c72ea686e59
> > >     (netfilter: netns nat: per-netns NAT table) renamed the
> > >     nat_table from __nat_table to nat_table without updating the
> > >     __RW_LOCK_UNLOCKED(__nat_table.lock).
> > >     
> > >     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > 
> > Applied to net-2.6, thanks Steven.
> > 
> > As Andrew mentioned this is a bug (albeit a "nano-bug" as you
> > called it :-) so I removed the Impact line in the commit
> > message when applying this.
> 
> Speaking of Impact: lines, is this a new fashion or what?
> 
> Looking at the ones which are already in official tree, they are either 
> trivially duplicating Subject: line, or effectively duplicating Subject: 
> line, or cover up for insufficiently informative (read: badly written) 
> Subject: line, or simply useless.

> 	Subject: sched: CPU remove deadlock fix
> 	Impact: fix possible deadlock in CPU hot-remove path
> 
> What prevented to write "Subject: sched: fix possible deadlock in CPU 
> hot-remove path"?

there are 655 Impact lines, right now, and we find them rather useful, for 
a multitude of reasons. The impact line is a secondary subject line in 
essence, describing the intended scope and practical impact of a change - 
and not describe the change itself. The advantages are:

 - they encourage a proper splitup of patches:

    - the more complex a patch is, the harder it is to write a proper 
      impact line for it. So when people complain to me that they find it 
      hard to describe the practical impact of a patch in a single line, i 
      ask them to split up the patch ;-)

 - they standardize and document impact analysis:

    - it helps -stable later on in filtering out fixes and ordering
      them by risk. We might not want to mark a commit as Cc: stable 
      straight away - but we want to describe the risk analysis we have 
      performed.

    - they also help filtering out the patches that go into -git versus 
      devel stuff.

 - they help bug analysis:

    - Impact lines make it abundantly clear what the intented scope of a 
      change was, on the first line of the commit. We had incidents in the 
      past where people bisected to a commit and were wondering whether a 
      change was intended to have side-effects or not - and the Impact 
      line made it clear that the side-effect was not intended.

    - they help bisection itself too: a couple of times i used it already 
      to home in on a suspected change that introduced a breakage. If 
      there's a material change in the middle of cleanups, it's hard to 
      see that in the changelogs immediately.

 - they ease review:

    - when a patch comes in that has an Impact line, i just look at the 
      impact line and match it up with what the patch actually does. If
      there's mismatch it raises a red flag. Subject lines and changelogs
      come from dozens and dozens of different authors, with different
      cultural and language backgrounds, with different levels of
      experience. It's much easier (and faster) to approach a patch with
      an impact line from the right angle.

    - they make it much harder to apply patches without a proper
      level of review. Creating a good impact line is a good last line of
      defense both at the submitter and at the applier level.

    - they make it clear to the patch submitter if i mis-judge a change. 
      They tell me when i create the wrong Impact line and i can 
      re-consider the change.

That is an overlapping but still different purpose from a subject line.

Subject lines are controlled by the 'what' and 'how' questions of a code 
change - while the Impact line is only controlled by the: 'risk'/'impact' 
aspect.

Subject lines are also often controlled by subsystem maintenance 
tradition, have various tags to distract from, etc. We try to match up 
subject lines close to the lkml subject were they were discussed - and 
only change them if they are really bad. That linkage is important. 
Appending and prepending impact information gets messy.

All kernel developers and maintainers who started using them (whom i 
talked to) found them rather useful - even if they had reservations about 
the seemingly duplicated subject line aspect in the beginning. I dont 
think you can judge this from an armchair - as you are only the reader of 
an impact line, not the creator of it. At least half of the good stuff 
happens while you active create them. Try it if you dont believe me ;-) In 
any case, you dont have to use it if you dont like it.

	Ingo