Design session, "Progressing of pending patch series"

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi All,

Please find the meeting notes as follows. Please feel free to correct 
anything.
Problem - Patches are not looked in reasonable time. This is an 
unsatisfactory situation for contributors especially if it involves 
effort for creating patches. This is not a new problem. New experts and 
bandwidth will not appear suddenly.
We want to collect thoughts about potential changes to the review 
process. Can we have a possible solution where some patches can be kept 
pending for months / years before it is eligible for review and  
committing ?

Bertrand - Situation improved in last few days, The maintainers can edit 
the commit message before committing instead of pushing back. It reduces 
the probability of further series. Now there is too much activity 
available on the mailing list.
Should we clean up the patches on the mailing list to distinguish 
between the patches to be reviewed vs not to be reviewed ?
George - Sometimes, there is not very actionable feedback or too little 
feedback. Lot of people are bypassing Andrew (unsure if the name is 
correct) for review. There are not enough reviewers. Sometime people 
unrelevant to the patch series are cc-ed. So the pipeline of a reviewer 
increases. How to encourage new people to review ?
Diego - What is the bottleneck from Andrew’s perspective

George - Andrew has taken time to review past patch series. Sometime 
security bugs are discovered quite late. Sometimes, reviewers are super 
busy , so they don’t review. We can have meetings with maintainers where 
people enjoy talking about review rather than writing. Then the reviewer 
can explain the bug.
Roger has been stepping up as reviewer to offload Andrew.

Bertrand - Eg In PCI passthrough, Roger was not available so everything 
was postponed. How to reduce bottlenecks? Can less experienced people 
review , but risks introducing bugs. Accept that some feature might go 
in with potential bugs (Feature marked unsupported), So end user can 
understand risk. Later the unsupported features can be marked supported 
when it is reviewed or tested.
Jan - Some person can review without being an explicit maintainer.

Bertrand - Some PCI passthrough issues are related to x86 . So we are 
blocked by the x86 review.  It is a case that Arm’s PCI passthrough 
feature cannot be upstreamed.
George - Can there be a mentorship program , where new reviewers will be 
paired with maintainers. And improve the review timelines.
Jan - Common code should be improved in all scenarios. What is in tree 
already should not deteriote. Some of the Arm's PCI passthrough changes 
introduces locking problem. Thus, we need to solve locking problem 
before Arm’s changes are introduced.
Bertrand - In an unsupported, you have a first version. If too much 
dependency is added in the first instance, then nothing progress.
Jurgen - Can we add experimental features with timeout. If no correction 
done on an experimental feature, then the feature can be removed
Roger - Who is going to do cleanup

Bertrand - All the new code is bounded by configuration. Xen code is 
modular. Easier to find what to remove.
George - Refactoring other parts of code will make it difficult

Jan - Some parts can still be scattered around. Can’t guard everything.

Bertrand - If 99% percent of code is modular, then probability of bugs 
is reduced.
Anthony - Makefile changes took lot of time.

Bertrand - Differentiate between critical / non critical patches so we 
can make faster progress.
Jan - When I rebase my tree, it took a day to solve problems introduced 
Makefile changes. Do not blindly merge.
Anthony had to take time to make out of tree work.

George - If we wait until a series is perfected, then there is a lower 
chance of bugs but lots of work/time. If we put things faster, then bugs 
may be introduced and breaks. Need to find a sweet mid spot.
Jurgen - Working on the central boot process is hitting everyone. 
Working on specific feature, only affects a limited set. Makefile 
changes (by Anthony) affect everybody.
On PCI issues, the locking issues need to be fixed first. If the locking 
is broken, then lot of side effects are introduced.
Bertrand - Spent lot of time discussion. We have an internal git branch 
with lots of downstream patches. Now we are upstreaming the initial 
features. Then x86 request came. No body looked at downstream gitlab repo.
Roger/Jan - No time to look at private gitlab repos.

Bertrand - How do we then fix things iteratively? Shouldn’t require 
fixing everything in the first instance.
Jan - Regarding checkin new features. There might be limitations , but 
bugs should not be checked in knowingly. FIXME should be used to fix 
severe bugs. Locking problem is a severe bug to be fixed before. No one 
had enough incentive to fix it. Unfortunate that Arm got affected.
EPAM is trying to fix the locking issues. Agree with Bertrand that 
things moved slowly. Should not be bad bugs involved.
Someone - How do we guys deal with scope creep ? When the scope of the 
initial work increases significantly
George - The work of refactoring of existing patch seems a lot of work 
(sometime Arm guys don’t have x86 hardware/knowledge). Who is going to 
do this ?
Jan - Ideally PCI should have been done with only Arm in mind.

George - All issue related to review bottleneck. If I need to review 
something, ping me on irc. Sometime, things slip. If there was system 
to  assign a ticket to person for review (on gitlab). Other projects 
have this philosophy
Jan - People shopuld be picking work, so they can do their best.

George - If the code is common with 6 mantainers, then sometimes I take 
time and review. In the meantime someone else reviewes.
Jan - Double review is good. Different issues can be seen by different 
person.
Bertrand - After one reviewers reviews the patch, should we send v2 or 
wait for another reviewer to see v1. Can’t see the status of review on a 
patch.
Jurgen - When I send a series for common code, Jan reviews , then I am 
confident that no one else will review.
Sometimes, Jan takes few days.

Jan - No problems with a v2 but it address all the open questions on v1.

George - Have mentoring problem for reviewers. Maintainers can mentor 
reviewer. Have a quiz to ask who will pick up the review.
Jurgen - Should I mention that I will do the review so that others will 
know.
George - Know what I need to review from the mailing list. Then I will 
look it.
Jurgen - Sometimes I review things which are related to linux. For other 
parts which I am not confident, I leave it to review.
George - People learn by reviewing code. If they have a mentor, then 
reviewers can do.
Jurgen - Reviewers may have a priority list for review.

Bertrand - Huge history in the mailing list. A rule that if my patch is 
more than 2 months old, resend it
George - Have a system which will track who is reviewing with what

Jan - In my experience, pings do not work at all. If I don’t have 
initial feedback why to send v2.
George - If a comment on v1 does not get addressed in 6 months, then 
what happens.
You can check with maintainers to give feedback in due course of time. 
Send a mail, review check it in two months and do it. Sometime series 
get blocked on a single maintainer. Should not be that someone becomes a 
bootleneck. We should offload things from people who are busy.
Jan - People should not be shy of looking at patches where maintainers 
have already commented. Objections should expire if they are not 
followed with concrete suggestion. So, other people can review and if 
others agree (without maintainers) , then commit it.
Bertrand - New reviewers take time to have confidence for review.

Jan - If I get review from new people, there is some level of mistrust 
if the patch series is intrusive. If the patch series is easy, then I 
can trust review from new people. If new people review frequently, then 
trust increases in due course of time. Also reviewers need to ask 
questions or spot problems on the patch. No blind review.
Kind regards,

Ayan