Copy-first platform additions (was: Re: Re: Proposal to add teeth to our Gerrit guidelines)
Am 05.11.19 um 20:23 schrieb Patrick Georgi:
On Tue, Nov 05, 2019 at 06:37:02PM +0100, Nico Huber wrote:
I mean "rubber-stamping of *huge* commits". That huge that it is obvious that no review happened, e.g. 1k+ LOC copy-pasta. Also, the guidelines say "This means you shouldn't +2 a patch just because you trust the author of a patch [...]" I didn't want to quote the whole paragraph, maybe I should have ;)
Some of the mega patches are copies of a predecessor chip (with the minimum amount of changes to integrate it in the build), that are then modified to fit the new chip.
Ack. I think that is a problem. If this procedure is intended, I think we should update our guidelines to reflect that.
The idea was precisely to ensure that changes between the generations are visible in the history.
That's nice for historians, but what about maintenance? There is nothing in the Git history if a piece of code was intended/validated/reviewed for a given chip (unless it was adapted which is the less common case).
That was considered an acceptable strategy for a long time (after we broke up 82801xx because fixing one variant broke another). We can certainly revisit that, but to me that change appears to have happened gradually and without much notice.
That's news to me. I didn't witness this break up. And it looks like afterwards, new chipsets were added with already patched code bases. The first unpatched copy I could find (didn't look very long) is soc/intel/braswell/.
Was there any public discussion that I could look up?
What I'd like to figure out most is how our Gerrit guidelines are supposed to be interpreted for a review of such a patch. I see no exception for not reviewing such patches and have no idea how one could review one. Also what would be the responsibilities of a reviewer after review?
Nico
On Wed, Nov 06, 2019 at 12:39:59PM +0100, Nico Huber wrote:
Some of the mega patches are copies of a predecessor chip (with the minimum amount of changes to integrate it in the build), that are then modified to fit the new chip.
Ack. I think that is a problem. If this procedure is intended, I think we should update our guidelines to reflect that.
I guess first we should get on the same page with regard to strategy. There's a bit of flip-flopping between extremes (code duplication vs. silently breaking stuff).
Once we're there, we should check if documentation can be improved (very likely yes).
That was considered an acceptable strategy for a long time (after we broke up 82801xx because fixing one variant broke another). We can certainly revisit that, but to me that change appears to have happened gradually and without much notice.
That's news to me. I didn't witness this break up. And it looks like
That break up was 2010, which I think is before your coreboot time. See commit 138be8315b63b0c8955159580d085e7621882b95
Was there any public discussion that I could look up?
That's not how the project worked back then.
What I'd like to figure out most is how our Gerrit guidelines are supposed to be interpreted for a review of such a patch. I see no exception for not reviewing such patches and have no idea how one could review one. Also what would be the responsibilities of a reviewer after review?
I suppose the criterion for such a commit is "is the diff to the original chipset code reasonable" (that is, changes names and stuff so it builds in parallel, but nothing else). Does that make sense?
Patrick
Hi again all,
I took part in (half of) a leadership meeting last night, and I was impressed by two things that I didn't expect:
1. Few people seem to take the might of Git into account. We have a tool that can significantly increase the efficiency of development and can help to preempt bugs. But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
2. The general acceptance of unreviewed code. Yes, one can try to argue that copy-pasted code was already reviewed. But in a different con- text and in a different time. Such argumentation also seems to assume that reviews are mostly about coding style and bikeshedding.
On a few occasions, I've already commented about these things on Gerrit. I'll now try to take the time to detail my concerns with above points in separate emails.
I really hope that discussing this will achieve something. I believe there are some $100k to save for the active coreboot community (albeit partial virtual $, as much of the work is done by volunteers).
Am 06.11.19 um 19:35 schrieb Patrick Georgi:
On Wed, Nov 06, 2019 at 12:39:59PM +0100, Nico Huber wrote:
Some of the mega patches are copies of a predecessor chip (with the minimum amount of changes to integrate it in the build), that are then modified to fit the new chip.
Ack. I think that is a problem. If this procedure is intended, I think we should update our guidelines to reflect that.
I guess first we should get on the same page with regard to strategy. There's a bit of flip-flopping between extremes (code duplication vs. silently breaking stuff).
I don't think this is about code duplication, at least to me that's a separate concern.
Nico
On Thu, Nov 07, 2019 at 12:05:44PM +0100, Nico Huber wrote:
- Few people seem to take the might of Git into account. We have a
Git is rather limited in some respects: For example it has no notion of a copy (except by using interesting merge hacks: create a branch where you rename, merge the two branches, keeping both copies in conflict resolution and you'll get a copy operation that git understands).
tool that can significantly increase the efficiency of development and can help to preempt bugs.
Interesting claim, but no: in the end it's up to developers. Git serves the function of a (rather stupid) librarian who keeps track of things so they don't get lost.
But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
As Stefan explained yesterday, the copy & adapt flow was established in part in response to issues with high velocity development overwhelming the project's review capacity.
There are two approaches, each with their upsides and downsides, but from my point of view the review capacity issue is a real issue, while "git can't track copies" is not: it's easier to improve git (I'd expect they accept patches) than to ramp up on reviewers (that aren't in the next step accused of rubber stamping commits).
One approach is to bring up a new chipset feature by feature. This might happen by bringing over code from another chipset that sufficiently similar, editing it and only then putting it up for review.
This means that every instance of the code will look slightly different (because devs actively have to take apart .c files and reassemble them as they reassemble the various features in the chipset), and every line of code will have to be reviewed. That's where things broke down.
On top of that, since every instance of the code will look oh-so-slightly different, it becomes harder to port fixes around along the lineage of a chipset.
The other approach is to replicate what the chip designers (probably) did: Take the previous generation, make a copy and modify, under the assumption that the old stuff isn't the worst place to start from.
This means that the first code drop for chip(X+1) in coreboot likely is more suitable for initializing chip(X) instead of chip(X+1), as that's what the later follow-up commits provide.
The main risk here seems to be that old stuff that isn't needed anymore isn't cut out of the code. On the upside, when applying transitive trust (that is: the assumption that the original chip(X+1) code isn't worse than the chip(X) code it came from), the main concern is looking what changed between the generations, reducing the load on the project during review.
You assert that the first approach I listed is superior to the other, but you ignore the issues we had with it.
And on the project management side it's a hard sell (to put it mildly) when you expect everybody to follow your preferred scheme (without acknowledging its downsides) while claiming that it's project policy (it's not: we don't have a policy on that).
We can decide to put up a policy, but we should be aware of the costs of whatever approach we mandate.
Patrick
Hi Patrick,
thank you for your response. I really appreciate that somebody helps to clear things up.
On 07.11.19 15:58, Patrick Georgi wrote:
On Thu, Nov 07, 2019 at 12:05:44PM +0100, Nico Huber wrote:
- Few people seem to take the might of Git into account. We have a
Git is rather limited in some respects: For example it has no notion of a copy (except by using interesting merge hacks: create a branch where you rename, merge the two branches, keeping both copies in conflict resolution and you'll get a copy operation that git understands).
Yeah, Git isn't perfect. But that doesn't mean we can't use what it provides, does it?
tool that can significantly increase the efficiency of development and can help to preempt bugs.
Interesting claim, but no: in the end it's up to developers. Git serves the function of a (rather stupid) librarian who keeps track of things so they don't get lost.
What are we bikeshedding here? That I said "[it] can" and should have said "[it] can be used to"?
But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
As Stefan explained yesterday, the copy & adapt flow was established in part in response to issues with high velocity development overwhelming the project's review capacity.
There are two approaches, each with their upsides and downsides, but from my point of view the review capacity issue is a real issue, while "git can't track copies" is not: it's easier to improve git (I'd expect they accept patches) than to ramp up on reviewers (that aren't in the next step accused of rubber stamping commits).
Is review capacity still an issue? From what you write it seems that it was back in 2012/2013 before I got really involved with the community. But during the last 3~4 years I witnessed the opposite. There were several occasions where I begged people to let me do a review and I was pushed away. At least in one case, I didn't even ask to break a big commit up. It was just decided that a review would be too much to bear for the submitter. So it would seem the capacity issue is on the other side:
Do we lack capacities to write reviewable patches?
If that is already the case, I fear it will turn into a race to the bottom. Less reviews, or
less thorough reviews => more bugs => less time
to write decent commits.
One approach is to bring up a new chipset feature by feature. This might happen by bringing over code from another chipset that sufficiently similar, editing it and only then putting it up for review.
This means that every instance of the code will look slightly different (because devs actively have to take apart .c files and reassemble them as they reassemble the various features in the chipset), and every line of code will have to be reviewed. That's where things broke down.
On top of that, since every instance of the code will look oh-so-slightly different, it becomes harder to port fixes around along the lineage of a chipset.
I agree, that is an issue I've seen, too. Though, without any guideline to keep older copies up to speed, we have that in any case (currently, everything is growing uncontrolled).
The other approach is to replicate what the chip designers (probably) did: Take the previous generation, make a copy and modify, under the assumption that the old stuff isn't the worst place to start from.
This means that the first code drop for chip(X+1) in coreboot likely is more suitable for initializing chip(X) instead of chip(X+1), as that's what the later follow-up commits provide.
The main risk here seems to be that old stuff that isn't needed anymore isn't cut out of the code.
That is one issue but I see a much bigger one...
On the upside, when applying transitive trust (that is: the assumption that the original chip(X+1) code isn't worse than the chip(X) code it came from),
... transitive trust is a fallacy. While the copied code isn't worse in terms of coding style and whatever poison the programming language has to offer, it can be worse for the new chip. If we don't keep a record and review what has to be changed for the new chip, more differences between old and new will be overlooked.
I can't prove it. But I assume that the effort to find overlooked details afterwards exceeds what we saved during the initial review.
the main concern is looking what changed between the generations, reducing the load on the project during review.
You assert that the first approach I listed is superior to the other, but you ignore the issues we had with it.
I'm not ignoring the past, sorry if I made it look like that. It just came as a surprise to me because I didn't witness any discussion of the review-capacity trouble. I have no idea, though, how to measure if we still have that problem. If reviewing turns out too slow, it's hard to tell, which side, author or reviewer, has to invest more time (if the authors know what they are doing, it's easier for them to speed things up, IMHO).
And on the project management side it's a hard sell (to put it mildly) when you expect everybody to follow your preferred scheme (without acknowledging its downsides) while claiming that it's project policy (it's not: we don't have a policy on that).
Well, I was told that we review the code before it's merged. IIRC, by you :) That we make exceptions due to lack of reviewing capacities, that I wasn't told. Later, our Gerrit guidelines were introduced and they show no sign of exceptions. I guess we should be more careful about what we write down and even more so about what we tell people in private without writing it down.
We can decide to put up a policy, but we should be aware of the costs of whatever approach we mandate.
Given that the code is often not readable anymore, lacks reason and reasoning, I believe that the costs of the copy-first approach are too high. Let's try something else?
Nico
On 07.11.19 12:05, Nico Huber wrote:
- Few people seem to take the might of Git into account. We have a tool that can significantly increase the efficiency of development and can help to preempt bugs. But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
Patrick already commented on two competing approaches to put the changes for a new chip into commits. I'd like to look closer at both and explain how I work with Git and why it makes it much much harder (at least for me) to work with copy-first additions.
What I call the copy-first approach ----------------------------------- 1. First commit copies existing code (with name changes only). n. Some commits change parts of the copied code that were identified to differ between the old and new chips. (n+1. Not a practice yet, but it was suggested to have a last commit to document things that aren't covered by the n changing commits; e.g. what didn't change but was tested)
The alternative step-by-step approach ------------------------------------- Each commit adds a chunk of code in a state that is supposed to be compatible with the new chip. Generally, smaller, cohesive commits are easier to review, of course. But there is no strict rule what the steps should be.
The major difference between the two approaches lies in what we know about the code chunks that stay the same for old and new chips. With the copy-first approach, we basically know nothing beside what we find in the history of an older chip. But we don't know if the code was left unchanged intentionally or if differences between the chips were overlooked. Also, nobody had an opportunity to review if it applies to the new chip. The n+1 commit might mitigate the documentation issue, but it's too late for review.
Sometimes people tell me that the copy-first approach makes it more visible what the author thinks changed between the old and new chips. I don't see yet, how we benefit from that (please help me).
With the step-by-step approach, we can easily document intentions in the commit messages, everything can be reviewed, and it's less likely to overlook changes between old and new chips.
That much about the initial addition of a new chip. But how does it look like later, when the first (non-reference) boards using the chip are added, or during maintenance? No matter what approach was applied, the code won't be perfect. And whenever somebody hits a bug or an incompatibility, or maybe just wonders why the code exists (e.g. when a datasheet doesn't mention anything related), people will have to investigate the reasons behind the code.
Personally, I make use a lot of the `git blame` command. It shows for each line of a file which commit touched that line last. And very often, the commit message provides some reasoning about the code. The step-by- step approach gives us the opportunity to document reasons, assumptions made and what was tested in the commit messages. With the copy-first approach, OTOH, this information is scattered. I'm not saying the reasons or test coverage would be different, just much harder to find, if documented at all. So for code that didn't change between old and new chips, in the best case, the copy-first approach with a documenting n+1 commit still increases costs maybe 5x to find the information. In the worst case much more.
Patrick asked to be aware of costs. Here is my view on this: The step- by-step approach is more expensive for the initial addition. But these are one-time costs. The copy-first approach is cheaper but more error- prone due to the lack of a thorough review. It does not only create further costs due to more bugs to fix, but also makes it harder to reason about the code, so bug-fixing costs increase further. Assuming the copy-first approach costs X and another X per year of maintenance, and we'd want to maintain a chip 3 years (IIRC, Intel sometimes sells chips for 5~7 years), we could spend 4*X instead on the step-by-step approach without losing anything.
Nico
On Fri, Nov 08, 2019 at 12:45:39AM +0100, Nico Huber wrote:
On 07.11.19 12:05, Nico Huber wrote:
- Few people seem to take the might of Git into account. We have a tool that can significantly increase the efficiency of development and can help to preempt bugs. But many people would accept a process that breaks Git benefits. Especially with the growth of the coreboot community, I believe this gains importance.
Patrick already commented on two competing approaches to put the changes for a new chip into commits. I'd like to look closer at both and explain how I work with Git and why it makes it much much harder (at least for me) to work with copy-first additions.
What I call the copy-first approach
- First commit copies existing code (with name changes only).
n. Some commits change parts of the copied code that were identified to differ between the old and new chips. (n+1. Not a practice yet, but it was suggested to have a last commit to document things that aren't covered by the n changing commits; e.g. what didn't change but was tested)
The alternative step-by-step approach
Each commit adds a chunk of code in a state that is supposed to be compatible with the new chip. Generally, smaller, cohesive commits are easier to review, of course. But there is no strict rule what the steps should be.
The major difference between the two approaches lies in what we know about the code chunks that stay the same for old and new chips. With the copy-first approach, we basically know nothing beside what we find in the history of an older chip. But we don't know if the code was left unchanged intentionally or if differences between the chips were overlooked. Also, nobody had an opportunity to review if it applies to the new chip. The n+1 commit might mitigate the documentation issue, but it's too late for review.
Sometimes people tell me that the copy-first approach makes it more visible what the author thinks changed between the old and new chips. I don't see yet, how we benefit from that (please help me).
With the step-by-step approach, we can easily document intentions in the commit messages, everything can be reviewed, and it's less likely to overlook changes between old and new chips.
That much about the initial addition of a new chip. But how does it look like later, when the first (non-reference) boards using the chip are added, or during maintenance? No matter what approach was applied, the code won't be perfect. And whenever somebody hits a bug or an incompatibility, or maybe just wonders why the code exists (e.g. when a datasheet doesn't mention anything related), people will have to investigate the reasons behind the code.
Personally, I make use a lot of the `git blame` command. It shows for each line of a file which commit touched that line last. And very often, the commit message provides some reasoning about the code. The step-by- step approach gives us the opportunity to document reasons, assumptions made and what was tested in the commit messages. With the copy-first approach, OTOH, this information is scattered. I'm not saying the reasons or test coverage would be different, just much harder to find, if documented at all. So for code that didn't change between old and new chips, in the best case, the copy-first approach with a documenting n+1 commit still increases costs maybe 5x to find the information. In the worst case much more.
I'd like to share my experience here with porting an intel/denverton board to coreboot.
During the first phase while I was discovering both the coreboot codebase and the board/chip I was often browsing through the different intel generation, comparing code that looked alike but with sometime small differences, or sometime totally different implementation for implementing similar features (I recall some ACPI code for instance).
So I can see why Nico thinks this approach cost more.
The first implementation was done based on a non public port by Intel (as the chip was not public anyway this is ... understandable ...) by the time the chip was released I had more knowledge and the denverton chip clearly had internals very similar to the Apollolake chip.
And Apollolake is basically the initial version for the common code in intel soc codebase, so when I contributed changes I pushed to reuse the common code and extend it where there where difference between denverton and apollolake.
I think that this approach while not being usable for all chip might be better than the plain copy-first as most code remain shared (and can be enabled progressively). And by remaining shared it means fix or improvement can benefit all the chip using the common code. There is obviously the risk of breaking old board while modifying the common code. Which probably mean we need more testing !
That was my though and small experience on the subject.
Julien VdG
Patrick asked to be aware of costs. Here is my view on this: The step- by-step approach is more expensive for the initial addition. But these are one-time costs. The copy-first approach is cheaper but more error- prone due to the lack of a thorough review. It does not only create further costs due to more bugs to fix, but also makes it harder to reason about the code, so bug-fixing costs increase further. Assuming the copy-first approach costs X and another X per year of maintenance, and we'd want to maintain a chip 3 years (IIRC, Intel sometimes sells chips for 5~7 years), we could spend 4*X instead on the step-by-step approach without losing anything.
Nico _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Looks like I'm too late to the party here, but I just read this and wanted to also come out in support of what Nico said. I think the endless copy&pasting that seems to have happened 5-10 years ago in coreboot left us with pretty awful maintainability consequences and we should (and I think we mostly are?) definitely move away from that. It's not just about correctness, it's also a huge obstacle for anyone trying to refactor some common interface that is used by hundreds of almost-identical board or chipset versions and actively hindering us from developing common code forward. Maybe it served a purpose at the time when the project was smaller and there were less people with time to review, but these days we are a pretty mature project with a lot of industry support (i.e. new boards and chipsets mostly get added by salaried vendor engineers rather than reverse-engineering hobbyists) and should be able to afford higher code quality and reuseability standards.
I think most of this is changing and has changed already (like Patrick said it happened gradually) -- these days most chipset code for both x86 and Arm is in src/soc/vendor/common, and most new mainboards use the variant system. Whenever I see a patch of someone copy&pasting a lot of code I definitely ask them to find a way to factor it out and make it common instead. But if there is still uncertainty about what the general right approach for new code is, I think we should decide (and maybe write it down somewhere) that it should be code reuse and not copy&paste.
I think most of this is changing and has changed already (like Patrick said it happened gradually) -- these days most chipset code for both x86 and Arm is in src/soc/vendor/common, and most new mainboards use the variant system. Whenever I see a patch of someone copy&pasting a lot of code I definitely ask them to find a way to factor it out and make it common instead. But if there is still uncertainty about what the general right approach for new code is, I think we should decide (and maybe write it down somewhere) that it should be code reuse and not copy&paste.
I think you hit on the key point here about writing down what you see as a better development model. For mainboard ports, the documentation that exists suggests finding a similar board, creating a new directory, and copying files from it: https://www.coreboot.org/Motherboard_Porting_Guide
-
David Hendricks -
Julien Viard de Galbert -
Julius Werner -
Nico Huber -
Nico Huber -
Patrick Georgi