Message-ID: <YTdswC0SvBWExoVk@lunn.ch>
Date:   Tue, 7 Sep 2021 15:44:32 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        patchwork-bot+netdevbpf@...nel.org,
        Rafał Miłecki <zajec5@...il.com>,
        vivien.didelot@...il.com, olteanv@...il.com, davem@...emloft.net,
        netdev@...r.kernel.org, rafal@...ecki.pl
Subject: Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x

On Mon, Sep 06, 2021 at 06:48:38PM -0700, Jakub Kicinski wrote:
> On Mon, 6 Sep 2021 02:48:34 +0200 Andrew Lunn wrote:
> > > not allowing a proper review to happen. So please, I am begging you, wait at
> > > least 12h, ideally 24h before applying a patch.  
> 
> The fixed wait time before applying would likely require more nuance.
> For example something like 0h for build fixed; 12h if reviewed by all
> area experts; 24h+ for the rest? Not counting weekends?
> 
> > 24 hours is too short. We all have lives outside of the kernel. I
> > found the older policy of 3 days worked well. Enough time for those
> > who had interest to do a review, but short enough to not really slow
> > down development. And 3 days is still probably faster than any other
> > subsystem.
> 
> It is deeply unsatisfying tho to be waiting for reviews 3 days, ping
> people and then have to apply the patch anyway based on one's own
> judgment.

I would skip the ping bit and just apply it. Unless you really think
it needs a deep review.

> Right now we make some attempts to delegate to "Needs ACK" state but
> with mixed result (see the two patches hanging in that state now).
> 
> Perhaps the "Plan to review" marking in pw is also putting the cart
> before the horse.

For me personally, the reason i like three days is that sometimes i'm
away in the wilderness, visiting friends, away on business, etc. I'm
not checking emails. So having to take some sort of action to say i'm
not going to take any action until later, just defeaters the point.

> Either way if we're expending brain cycles on process changes it would
> be cool to think more broadly than just "how long to set a timer for".

One observation is that netdev drivers are very siloed. A vendor
driver rarely gets reviewed by another vendor. I think reviews are
mostly made by vendor neutral people, and they look for specific
things. I'm interested in phy, ethtool, and anything which looks like
it could be adding a new KAPI of some sort. There are people who
trigger on the keyword XDP, or BPF, devlink etc. Some areas of netdev
do tend to get more reviews than others. Again, my areas of interest,
phys, DSA, ethtool. The core stack gets reviewed by Eric, routing by
David, and i'm sure there are others in parts i don't take an interest
and i've not noticed.

If a patch is from somebody who is not the maintainer of a siloed
driver, and the maintainer is active, then a review is more likely to
happen.

So some sort of model could be made of this, to predict if a patchset
is likely to get reviewed, if time is allowed for the reviewer to
actually do the review. I don't know if a mental model is sufficient,
for the two people who are merging patches, or some tools could be
produced to help a little. Maybe just simple things like, is the
poster of the patch series the maintain of the driver it applies
to. Maybe some keyword matching? Maybe Sasha Levin can run a machine
learning system over the last few years of the netdev archive to train
a model which will product if a patchset will get reviewed? That would
be applicable outside of netdev, it could be a useful feature in
general. Maybe it is a research project Julia Lawall at Inria could
give to a PhD student?

  Andrew

Powered by blists - more mailing lists