LWN: Comments on "KS2011: Patch review"

KS2011: Patch review

wookey — 2011-11-08T18:58:20+00:00

If apple want to attack the linux kernel directly then bring it on. The kernel has powerful friends these days like IBM, OIN, and erm, well, pretty much everybody. I'd like to think that any serious attack would be about as good for Apple as it was for SCO and Blackboard. Obviously a patent-based attack would be far less stupid than SCO's copyright-based attack, but ultimately we can remove anything which is found to be infringing, and the exposure of the madness inherent in the system can only do us good.

KS2011: Patch review

badcow — 2011-11-01T18:55:09+00:00

There's no concern over Apple's desire to destroy the Android? I'm not sure that threat was just targeted at devices running it.

KS2011: Patch review

BenHutchings — 2011-10-26T22:32:46+00:00

According to my notes, Linus said that a second Signed-off-by implies only Acked-by; and the person adding it could add Reviewed-by if they want.

KS2011: Patch review

a0273185@ti.com — 2011-10-26T15:37:47+00:00

ION provides memory allocation and buffer sharing using file descriptors. The allocations come from carveouts.

dmabuf just deals with buffer sharing.

I imagine CMA + dmabuf would displace ION.

KS2011: Patch review

tytso — 2011-10-26T03:32:46+00:00

Let's consider when a Signed-off-by: should appear.

(1) when a patch is written by the original author and submitted for consideration. At that point, the author had better be convinced that it is Correct and Sane, since he or she is putting their name and reputation on the line with that patch. In some cases patches are submitted for comments, and in that case sometimes the signed-off-by line is not present (because the author knows some changes might be necessary), or to make the point that the patch really isn't suitable for inclusion, might even have a NOT-Signed-off-by: line to make that point clear.

(2) when the patch is copied from e-mail into a git repository. If the person with the git repository then sends a pull request to Linus, it had better mean that she has put her name and reputation on the line, and is confident that the patch is good. It may not mean that the person has reviewed it; it may be that he is depending on his reviewers or sub-maintainers to have done a good job. But ultimately, it is the maintainer's reputation which is on the line. If the maintainer isn't sure about a sub-maintainer's competence in review and good taste, then she should be doing spot checks or even review every single commit before accepting a pull from the sub-maintainer --- or not stop pulling form the sub-maintainer altogether.

This also implies that in practice, a commit should generally only have two signed-off-by lines. There is an exception, which is if there is a sub-maintainer who isn't using git, so a patch gets forwarded outside of git an additional time. An example of this Andrew Morton's mm tree, which is maintained as a quilt patch series outside of git. But in general, patches are reviewed on e-mail, and once they hit git, they're not going to accumulate any more signed-off-by lines. As those git commits get merged into more senior trees, the maintainer who accepts the pull request is effectively putting their imprimatur on them. So they had better have reviewed the patch, or be willing to put their reputation on the line based on their trust of their sub-maintainers.

KS2011: Patch review

dlang — 2011-10-25T18:01:17+00:00

this is the sort of thing I was thinking about when I was saying that things just needed to run, not implement everything.

I am thinking that the interfaces needed to just make things 'run' on a kernel.org kernel is much smaller than what people have been trying to get submitted. And then once this smaller set is in place, things can be worked on much more easily.

while I have your attention, did you see Neil Brown's proposal for a userspace wakelock-type daemon? how hard would it be to modify the Android userspace to work with his proposal? As I understand it, there's only one daemon that actually talks to the kernel, everything else in the Android userspace talks to that daemon.

KS2011: Patch review

xav — 2011-10-25T09:31:41+00:00

Interesting. How does ION relate to the DMA buffer sharing mechanism [1] currently in discussion ?

[1] https://lwn.net/Articles/462524/

KS2011: Patch review

swetland — 2011-10-25T08:23:45+00:00

Userspace should fail over to dropping all logs on the floor. Not useful but it should work.

We're still getting back online after the kernel.org downtime, so I don't have a handy URL for browseable source. Before kernel.org went down the ION kernel side sources were being developed in the open in the kernel repositories at android.git.kernel.org.

Hopefully it'll all be sorted out in the near future alongside the ICS userspace source release.

KS2011: Patch review

dlang — 2011-10-25T08:17:49+00:00

is the logger driver really needed for the userspace to run? could you point me to documentation on it?

as for ION, since there is no code available yet, it's hard to talk about it.

KS2011: Patch review

swetland — 2011-10-25T08:08:09+00:00

I'd need to look through things, but I suspect the answer is "not many", though some of them are pretty important (at least to Android).

Some of the notable ones off the top of my head:
- wakelocks
- binder (IPC subsystem) -- /dev/binder -- self-contained, in theory one could replace it with an entirely userspace implementation, in practice probably a lot of work
- logger driver -- /dev/logs/* -- multiple ring buffers for logging state without a lot of context switch overhead -- back when running on virtually tagged ARM9s this was very useful
- ION -- /dev/ion -- a unified approach to buffer management and sharing between display, GPU, camera, codecs, etc, new in Ice Cream Sandwich

Where possible we try to use standard interfaces -- for example, the power supply framework for battery charging and status, a lot of assorted interfaces via /sys, etc.

KS2011: Patch review

dlang — 2011-10-25T08:00:24+00:00

not counting hardware drivers for the moment, is there a list of what interfaces would need to get added to the kernel.org kernel to get Android userspace to run on it?

and then of those interfaces, which ones need to actually be something other than a stub?

for example, wakelocks could probably be a stub, but an inter-process communications would have to actually work (I seem to remember reading that Android had a kernel assisted IPC protocol)

KS2011: Patch review

swetland — 2011-10-25T07:33:51+00:00

I'd love that too!

To be very clear, we (Google/Android) are not opposed to some level of compromise, incremental shifts, and migration of APIs. We just find the "no you should throw it all away and rewrite your userspace to be different" approach to be something we're not interested in doing.

I suspect there's a middle ground somewhere between "put all of the Android changes in the kernel unmodified right now" and "delete them forever because they suck". Here's hoping we find it.

KS2011: Patch review

dlang — 2011-10-25T03:55:23+00:00

I would love to have the a ability to put a kernel.org kernel on my devices that shipped with android. I see far too much work going on with people writing drivers for (in kernel.org terms) ancient kernels.

in many cases there is a driver for a device in a new kernel, but since that's not an 'android' kernel, people spend a lot of time duplicating or backporting drivers.

I'm not saying that the kernel.org kernel needs to accept all the implementation details of the android kernels, but they do need to accept enough that the interfaces can be made available and the result at least run.

in the case of wakelocks, on my tablet I don't have all the different weird things going on that can happen on a phone, so I would even be happy with a noop version of wakelocks that didn't actually do anything beyond allowing the android userspace to run without complaining.

KS2011: Patch review

neilbrown — 2011-10-25T00:59:23+00:00

> That led to some discussion of the relative meaning and merit of Reviewed-by and Signed-off-by. The latter is a much stronger statement in the end, and, it was agreed, carries more weight.

This makes no sense to me. As defined in SubmittingPatches, the Signed-off-by: line says nothing about the quality of the patch, only the origin.
It represents a "Developer's Certificate of Origin" and is meant to address questions of copyright, not questions of value.

So it seems to me that if the original statement is true, then people are interpreting SOB wrongly.

SubmittingPatches gives a "Reviewer's statement of oversight" which defines what Reviewed-by: is meant to mean, and it sounds like it should carry a whole lot more weight...

Do we need to revise SubmittingPatches ??