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 <[email protected]>
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 <[email protected]>
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),
};
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 <[email protected]>
> 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 <[email protected]>
>
> 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
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
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 <[email protected]>
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 <[email protected]>
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,
};
On Wed, 10 Dec 2008 15:06:00 -0500 (EST)
Steven Rostedt <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
>
> 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?
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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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
From: Steven Rostedt <[email protected]>
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 <[email protected]>
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!
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 <[email protected]>
>
> 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?
On Tue, 16 Dec 2008 04:10:39 +0300 Alexey Dobriyan <[email protected]> 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 <[email protected]>
> >
> > 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.
From: Alexey Dobriyan <[email protected]>
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 <[email protected]>
> >
> > 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...
Em Mon, Dec 15, 2008 at 05:19:35PM -0800, Andrew Morton escreveu:
> On Tue, 16 Dec 2008 04:10:39 +0300 Alexey Dobriyan <[email protected]> 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
* Alexey Dobriyan <[email protected]> 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 <[email protected]>
> >
> > 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