Process to push changes to include/linux/types.h

Process to push changes to include/linux/types.h

[Eric Paris]

A patch was posted a bit ago by agruen which made a change to
include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
this new type to userspace.

http://marc.info/?l=linux-kernel&m=128316627912457&w=2

Everyone seemed to agree the patch was a good idea and was correct.  At
the moment this change only really affects network code, but I would
very much like to make use of this change in the notification tree.
Dave Miller did not apply the patch because "Someone has to first add
the types to linux/types.h, and that doesn't go through my tree."

http://marc.info/?l=linux-kernel&m=128634544524035&w=2

I'm a little stuck as to the right path forward.  I normally would have
had no qualms about adding __aligned_u64 to types.h in the notification
tree and pushing it to Linus next go-round and then the net tree could
convert and potentially drop the old aligned_u64 type (but again that
would be outside the net tree).  Since Dave isn't willing to add the
type and I don't want to get called too many bad names, I figured I
should try to find if there is some better way, maintainer, or tree who
should be adding this new type.

Who needs to sign off on a new type in types.h?  Who should add it?
Should I just ram it on in there myself, take any flames that come
along, and then let net finish their cleanups after I've been charred?
Any suggestions on the best course of action would be appreciated.

-Eric

Re: Process to push changes to include/linux/types.h

[Andi Kleen]

On Thu, Oct 14, 2010 at 03:34:52PM -0400, Eric Paris wrote:
> A patch was posted a bit ago by agruen which made a change to
> include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
> this new type to userspace.

I just ran into this problem (aligned_u64 not being exposed to user space),
so yes I definitely welcome that change.


> 
> http://marc.info/?l=linux-kernel&m=128316627912457&w=2
> 
> Everyone seemed to agree the patch was a good idea and was correct.  At
> the moment this change only really affects network code, but I would
> very much like to make use of this change in the notification tree.
> Dave Miller did not apply the patch because "Someone has to first add
> the types to linux/types.h, and that doesn't go through my tree."

When in doubt send it to Andrew I guess. I see you already did.

-Andi

Re: Process to push changes to include/linux/types.h

[Andrew Morton]

On Thu, 14 Oct 2010 15:34:52 -0400
Eric Paris <eparis@redhat.com> wrote:

> A patch was posted a bit ago by agruen which made a change to
> include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
> this new type to userspace.
> 
> http://marc.info/?l=linux-kernel&m=128316627912457&w=2
> 
> Everyone seemed to agree the patch was a good idea and was correct.  At
> the moment this change only really affects network code, but I would
> very much like to make use of this change in the notification tree.
> Dave Miller did not apply the patch because "Someone has to first add
> the types to linux/types.h, and that doesn't go through my tree."
> 
> http://marc.info/?l=linux-kernel&m=128634544524035&w=2
> 
> I'm a little stuck as to the right path forward.  I normally would have
> had no qualms about adding __aligned_u64 to types.h in the notification
> tree and pushing it to Linus next go-round and then the net tree could
> convert and potentially drop the old aligned_u64 type (but again that
> would be outside the net tree).  Since Dave isn't willing to add the
> type and I don't want to get called too many bad names, I figured I
> should try to find if there is some better way, maintainer, or tree who
> should be adding this new type.
> 
> Who needs to sign off on a new type in types.h?  Who should add it?
> Should I just ram it on in there myself, take any flames that come
> along, and then let net finish their cleanups after I've been charred?
> Any suggestions on the best course of action would be appreciated.
> 

The usual approach here is someone sends it to me and I send it to
Linus ;)

If the change is simple, obviously safe and is needed in two or more
subsystem trees then I'll usually sneak it into mainline late in -rc,
simply to make everyone's life easier.  Of course, you could both agree
to merge the same patch into local trees and I assume that git will
sort it all out.

For this particular patch I'd suggest it be split into two: one adds
the new __aligned_u64 and friends.  The second patch kills off
aligned_u64 and friends.  I'd say the first four-liner would then be
safe for immediate merge and the cleanup patch can go in any old time.



Regarding the patch itself: it uses open-coded
__attribute__((aligned(...))), however we have the __aligned(...)
helpers in compiler.h.

I'm always a bit ambivalent about those helpers (__packed, etc). 
They're not a very kernely thing to do, but the gcc __attribute__
syntax really is a mouthful.

And adding a compiler.h dependency to the shared-with-userspace types.h
may not be practical or safe, dunno.


So if this works, I'd suggest preparing the simple four-liner with the
intention of an immediate merge.

Re: Process to push changes to include/linux/types.h

[Jan Engelhardt]

On Thursday 2010-10-14 21:54, Andrew Morton wrote:
>
>> A patch was posted a bit ago by agruen which made a change to
>> include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
>> this new type to userspace.
>> [...]
>> I'm a little stuck as to the right path forward.  I normally would have
>> had no qualms about adding __aligned_u64 to types.h in the notification
>> tree and pushing it to Linus next go-round and then the net tree could
>> convert and potentially drop the old aligned_u64 type (but again that
>> would be outside the net tree).  Since Dave isn't willing to add the
>> type and I don't want to get called too many bad names
>
>The usual approach here is someone sends it to me and I send it to
>Linus ;)

We tinkered on types.h before, with the change originating in the Netfilter
subtree, and nobody, not even Dave, complained. (See v2.6.24-6165-gc82a5cb)

Re: Process to push changes to include/linux/types.h

[Eric Paris]

On Thu, 2010-10-14 at 23:26 +0200, Jan Engelhardt wrote:
> On Thursday 2010-10-14 21:54, Andrew Morton wrote:
> >
> >> A patch was posted a bit ago by agruen which made a change to
> >> include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
> >> this new type to userspace.
> >> [...]
> >> I'm a little stuck as to the right path forward.  I normally would have
> >> had no qualms about adding __aligned_u64 to types.h in the notification
> >> tree and pushing it to Linus next go-round and then the net tree could
> >> convert and potentially drop the old aligned_u64 type (but again that
> >> would be outside the net tree).  Since Dave isn't willing to add the
> >> type and I don't want to get called too many bad names
> >
> >The usual approach here is someone sends it to me and I send it to
> >Linus ;)
> 
> We tinkered on types.h before, with the change originating in the Netfilter
> subtree, and nobody, not even Dave, complained. (See v2.6.24-6165-gc82a5cb)

I'm doing the separation and will send part 1 to akpm in a bit.  Make it
easy and make sure noone gets upset   :)

-Eric

Re: Process to push changes to include/linux/types.h

[Andrew Morton]

On Thu, 14 Oct 2010 23:26:34 +0200 (CEST)
Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Thursday 2010-10-14 21:54, Andrew Morton wrote:
> >
> >> A patch was posted a bit ago by agruen which made a change to
> >> include/linux/types.h changing aligned_u64 to __aligned_u64 and exposing
> >> this new type to userspace.
> >> [...]
> >> I'm a little stuck as to the right path forward.  I normally would have
> >> had no qualms about adding __aligned_u64 to types.h in the notification
> >> tree and pushing it to Linus next go-round and then the net tree could
> >> convert and potentially drop the old aligned_u64 type (but again that
> >> would be outside the net tree).  Since Dave isn't willing to add the
> >> type and I don't want to get called too many bad names
> >
> >The usual approach here is someone sends it to me and I send it to
> >Linus ;)
> 
> We tinkered on types.h before, with the change originating in the Netfilter
> subtree, and nobody, not even Dave, complained.

It doesn't matter much at all what tree a change goes through.  What
matters more is that the appropriate people know about and see the
change.

For example, I never even knew that aligned_u64 and friends existed (it
got secretly merged via the netfilter tree, apparently).  So when I
review code (and I review a lot of code), I don't think to nag people
if they open-code it.  <greps>.  That doesn't seem to have happened yet.

> (See v2.6.24-6165-gc82a5cb)

hm, what does that mean.

Re: Process to push changes to include/linux/types.h

[Randy Dunlap]

On Thu, 14 Oct 2010 14:37:23 -0700 Andrew Morton wrote:

> > (See v2.6.24-6165-gc82a5cb)
> 
> hm, what does that mean.
> --

v2.6.24 + 6165 changesets, with c82a5cb being the last commit:

> git show c82a5cb | more
commit c82a5cb8b2b2ce15f1fb8add6772921b72da5943
Author: Jan Engelhardt <jengelh@computergmbh.de>
Date:   Thu Jan 31 03:57:36 2008 -0800

    linux/types.h: Use __u64 for aligned_u64

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: Process to push changes to include/linux/types.h

[Jan Engelhardt]

On Thursday 2010-10-14 23:37, Andrew Morton wrote:
>> >The usual approach here is someone sends it to me and I send it to
>> >Linus ;)
>> 
>> We tinkered on types.h before, with the change originating in the Netfilter
>> subtree, and nobody, not even Dave, complained.
>
>It doesn't matter much at all what tree a change goes through.  What
>matters more is that the appropriate people know about and see the
>change.
>
>For example, I never even knew that aligned_u64 and friends existed (it
>got secretly merged via the netfilter tree, apparently).

I would be interested in knowing whether you - in whichever subsystems
you happen to be active - would even need aligned_u64. Right now,
the only users seem to be PPP and scsi_tgt besides Netfilter.

>So when I review code (and I review a lot of code), I don't think to nag
>people if they open-code it. <greps>. That doesn't seem to have happened yet.

When these typedefs were introduced, I am sure that any open-coded
invocations that the new type sought to replace was adjusted.

>> (See v2.6.24-6165-gc82a5cb)
>
>hm, what does that mean.

Eh, it's a git commit identifier. When people throw around with these
(or their longer, 40-char variants), it is suggested to use `git log
-1 -p v2.6.24-6165-gc82a5cb` to see the details.
Or equivalent ways, such as visiting
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=v2.6.24-6165-gc82a5cb

Re: Process to push changes to include/linux/types.h

[Andrew Morton]

On Thu, 14 Oct 2010 23:50:34 +0200 (CEST)
Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Thursday 2010-10-14 23:37, Andrew Morton wrote:
> >> >The usual approach here is someone sends it to me and I send it to
> >> >Linus ;)
> >> 
> >> We tinkered on types.h before, with the change originating in the Netfilter
> >> subtree, and nobody, not even Dave, complained.
> >
> >It doesn't matter much at all what tree a change goes through.  What
> >matters more is that the appropriate people know about and see the
> >change.
> >
> >For example, I never even knew that aligned_u64 and friends existed (it
> >got secretly merged via the netfilter tree, apparently).
> 
> I would be interested in knowing whether you - in whichever subsystems
> you happen to be active - would even need aligned_u64. Right now,
> the only users seem to be PPP and scsi_tgt besides Netfilter.
> 

Stranger things have happened.

include/linux/net_dropmon.h:net_dm_config_entry can perhaps be
converted btw.

Re: Process to push changes to include/linux/types.h

[Linus Torvalds]

On Thu, Oct 14, 2010 at 2:50 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
>>> (See v2.6.24-6165-gc82a5cb)
>>
>>hm, what does that mean.
>
> Eh, it's a git commit identifier. When people throw around with these
> (or their longer, 40-char variants), it is suggested to use `git log
> -1 -p v2.6.24-6165-gc82a5cb` to see the details.

[ I'm sure you know this, but just for any innocent non-git
by-standers out there ]

Well, technically it's more than just a git ID. It actually has some
descriptive value, and it's generated by "git descibe".

In something like the above, the only part git cares about in this
case is the (shortened) SHA1 hash, ie the "c82a5cb" part. Everything
else is just a mostly human-readable description, which boils down to
"based on v2.6.24, there's 6165 additional commits, and git commit
c82a5cb is where you end up". So the output is designed to (a) give
some helpful information even for intermediate places that aren't
exact tagged releases and (b) also work with other SCM's (that "-g"
part there is for "git" - but scripts/setlocalversion is able to give
somewhat reasonable output for hg and svn too.

Extended background for non-git people: git _internally_ only uses the
160-bit SHA1 (which in its full ASCII form is 40 hex characters). But
because that is so human-unfriendly, there are various human-readable
ways to express it:

 - Just a shortened unique version, usually 7-12 hex characters. 12
hex characters is already unique in practice for pretty much any
reasonable project, and is much easier to write.

   I tend to use the 12-character short version in commit messages,
for example. The full 40-character SHA1 makes it hard to do any sane
line breaks with in the commit message.

 - named tags or branches ("v2.6.36-rc7", or "master" etc)

 - The <random human-readable noise> + "-g" + <shortened SHA1> thing
described above (output by "git describe", and on input git ignores
the human-readable part), where the human-readable part tries to give
a release as a base for it.

 - a "git revision expression", where you can mix and match SHA1's or
named tags/branches, and their parenthood relationsips.

So that "v2.6.24-6165-gc82a5cb" could be expressed as

 - c82a5cb8b2b2ce15f1fb8add6772921b72da5943 (full SHA1)

 - c82a5cb8b2b2 (abbreviated to 12 hex chars)

 - v2.6.25-rc1~1089^2~98

where that last one is saying "you can reach it from v2.6.25-rc1 by
walking 1089 parents down, then taking the second parent in that
merge, and then walking 98 parents down again".

It's worth noting that the "v2.6.24-6165" - while human-readable and
thus useful - is technically meaningless. Since development isn't a
straight line, "6165 commits after 2.6.24" is really not a
well-defined point. It tells you _something_ - namely "not really very
close to plain 2.6.24" - but there may well be many places in the
history that are 6165 commits away from 2.6.24.

Which is why git really just ignores that human-readable part on input.

In contrast, the "v2.6.25-rc1~1089^2~98" expression is actually
well-defined. There is no ambiguity there, but it's also obviously not
really all that human-readable.

                              Linus

Re: Process to push changes to include/linux/types.h

[Jan Engelhardt]

On Friday 2010-10-15 00:13, Linus Torvalds wrote:
>
>Extended background for non-git people: git _internally_ only uses the
>160-bit SHA1 (which in its full ASCII form is 40 hex characters). But
>because that is so human-unfriendly, there are various human-readable
>ways to express it.
>[...]
>   I tend to use the 12-character short version in commit messages,
>for example. The full 40-character SHA1 makes it hard to do any sane
>line breaks with in the commit message.

I tend to use describe --tags (rev expr) output; unlike 12-char
versions, they cannot become ambiguous at any time. If git also used
the initial portion of regular describes ("v2.6.24-1234-"), it would
be similarly strengthened. Yes, I'm just theoretically projecting
what happens if \lim_{time,hackers -> \infty}...

>It's worth noting that the "v2.6.24-6165" - while human-readable and
>thus useful - is technically meaningless. Since development isn't a
>straight line, "6165 commits after 2.6.24" is really not a
>well-defined point.

It could be - in totally linear developments. (Postgresql, anyone?)

>In contrast, the "v2.6.25-rc1~1089^2~98" expression is actually
>well-defined. There is no ambiguity there, but it's also obviously not
>really all that human-readable.

I beg to differ. It tells you that the certain commit was included
for v2.6.25-rc1. That's much more telling than v2.6.24-1234-gabcdef1.
Especially when a branch has not been recently merged, it could be
showing v2.6.22-9876-gxxx or anything further back in time.


Jan

Abbrevieated SHA1s (Was: Re: Process to push changes to include/linux/types.h)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

Hi Linus,

On Thu, 14 Oct 2010 15:13:27 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> Extended background for non-git people: git _internally_ only uses the
> 160-bit SHA1 (which in its full ASCII form is 40 hex characters). But
> because that is so human-unfriendly, there are various human-readable
> ways to express it:
> 
>  - Just a shortened unique version, usually 7-12 hex characters. 12
> hex characters is already unique in practice for pretty much any
> reasonable project, and is much easier to write.

I have had one case I hit where an abbreviated SHA1 (which may have been
only 7 hex chars) was unique in your tree, but not in linux-next :-(  I
think it was unique after one more character, though.   Having this
happen was a pain, because git reacted badly to being given a non-unique
SHA1 to look up.  i.e. it did *not* say the expected "this sha1 is not
unique".   However, now it does, so things are getting better.  It would
probably be nice if it told you the "matching" objects ...

So I would support using (at least) 12 char abbreviations in commit
messages (and any other long lived context).
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: Process to push changes to include/linux/types.h

[Andi Kleen]

Jan Engelhardt <jengelh@medozas.de> writes:

> I would be interested in knowing whether you - in whichever subsystems
> you happen to be active - would even need aligned_u64. Right now,
> the only users seem to be PPP and scsi_tgt besides Netfilter.

Using aligned_u64 is good practice to avoid problems with the 
32bit/64bit compat layer. I would recommend it to anyone 
adding a new user space interface passing a 64bit value.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: Process to push changes to include/linux/types.h

[Andrew Morton]

On Fri, 15 Oct 2010 10:22:49 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> Jan Engelhardt <jengelh@medozas.de> writes:
> 
> > I would be interested in knowing whether you - in whichever subsystems
> > you happen to be active - would even need aligned_u64. Right now,
> > the only users seem to be PPP and scsi_tgt besides Netfilter.
> 
> Using aligned_u64 is good practice to avoid problems with the 
> 32bit/64bit compat layer. I would recommend it to anyone 
> adding a new user space interface passing a 64bit value.
> 

What "problems" does it prevent when used for this?

Re: Process to push changes to include/linux/types.h

[Andreas Gruenbacher]

On Friday 15 October 2010 11:01:34 Andrew Morton wrote:
> On Fri, 15 Oct 2010 10:22:49 +0200 Andi Kleen <andi@firstfloor.org> wrote:
> 
> > Jan Engelhardt <jengelh@medozas.de> writes:
> > 
> > > I would be interested in knowing whether you - in whichever subsystems
> > > you happen to be active - would even need aligned_u64. Right now,
> > > the only users seem to be PPP and scsi_tgt besides Netfilter.
> > 
> > Using aligned_u64 is good practice to avoid problems with the 
> > 32bit/64bit compat layer. I would recommend it to anyone 
> > adding a new user space interface passing a 64bit value.
> > 
> 
> What "problems" does it prevent when used for this?

64-bit values align to 4-byte boundaries on some 32-bit architectures like x86 
and to 8-byte boundaries on 64-bit architetures.  The new __aligned_64 type 
enforces 8-byte alignment and so structs containing __aligned_64 values have 
the same alignment on 32-bit and 64-bit architectures.  No conversions are 
necessary between 32-bit user-space and a 64-bit kernel.

Andreas

Re: Process to push changes to include/linux/types.h

[Andi Kleen]

> 64-bit values align to 4-byte boundaries on some 32-bit architectures like x86 
AFAIK it's only on x86, no other architecture made this mistake in their
32bit ABI. But of course x86 is kind of important ...

> and to 8-byte boundaries on 64-bit architetures.  The new __aligned_64 type 
> enforces 8-byte alignment and so structs containing __aligned_64 values have 
> the same alignment on 32-bit and 64-bit architectures.  No conversions are 
> necessary between 32-bit user-space and a 64-bit kernel.

Rest looks good and could be put into Andrew's comment.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: Process to push changes to include/linux/types.h

[Andreas Gruenbacher]

On Friday 15 October 2010 16:26:17 Andi Kleen wrote:
> > 64-bit values align to 4-byte boundaries on some 32-bit architectures like
> > x86 
> AFAIK it's only on x86, no other architecture made this mistake in their
> 32bit ABI. But of course x86 is kind of important ...

I don't know of any other examples; all the architectures I tried "got it 
right" except x86.

Andreas

Re: Process to push changes to include/linux/types.h

[Jan Engelhardt]

On Friday 2010-10-15 16:44, Andreas Gruenbacher wrote:
>On Friday 15 October 2010 16:26:17 Andi Kleen wrote:
>> > 64-bit values align to 4-byte boundaries on some 32-bit architectures like
>> > x86 
>> AFAIK it's only on x86, no other architecture made this mistake in their
>> 32bit ABI. But of course x86 is kind of important ...
>
>I don't know of any other examples; all the architectures I tried "got it 
>right" except x86.

I don't think x86 has done anything wrong. It processes stuff in
32-bit chunks, so why should one waste space for aligning a 64-bit
entity to 8? On sparcv9, a long double also has an alignment
smaller than its size.

If you don't like that, you can always patch up gcc, because
alignment is after all a decision made by the compiler. In that
regard, it might be worth checking out the gcc machine description
files and find out why people specified an alignment of 8 for
uint64_t anything but i?86.

Re: Process to push changes to include/linux/types.h

[Jan Engelhardt]

On Friday 2010-10-15 10:22, Andi Kleen wrote:
>Jan Engelhardt writes:
>
>> I would be interested in knowing whether you - in whichever subsystems
>> you happen to be active - would even need aligned_u64. Right now,
>> the only users seem to be PPP and scsi_tgt besides Netfilter.
>
>Using aligned_u64 is good practice to avoid problems with the 
>32bit/64bit compat layer. I would recommend it to anyone 
>adding a new user space interface passing a 64bit value.

Well, new interfaces in networking are using netlink, which has its own 
unlucky ideas of alignment.