[RFC] docs: netdev: encourage reviewers

[RFC] docs: netdev: encourage reviewers

[Jakub Kicinski]

Add a section to our maintainer doc encouraging reviewers
to chime in on the mailing list.

The questions about "when is it okay to share feedback"
keep coming up (most recently at netconf) and the answer
is "pretty much always".

The contents are partially based on a doc we wrote earlier
and shared with the vendors (for the "driver review rotation").

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: andrew@lunn.ch
CC: jesse.brandeburg@intel.com
CC: sd@queasysnail.net
CC: horms@verge.net.au

Sending as RFC for early round of reviews before I CC docs@
and expose this to potentially less constructive feedback :)
---
 Documentation/process/maintainer-netdev.rst | 43 +++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
index 09dcf6377c27..d5cbcfd44cf8 100644
--- a/Documentation/process/maintainer-netdev.rst
+++ b/Documentation/process/maintainer-netdev.rst
@@ -441,6 +441,49 @@ in a way which would break what would normally be considered uAPI.
 new ``netdevsim`` features must be accompanied by selftests under
 ``tools/testing/selftests/``.
 
+Reviewer guidance
+-----------------
+
+Reviewing other people's patches on the list is highly encouraged,
+regardless of the level of expertise. Code reviews not only help
+the maintainers but also help reviewers themselves to learn and
+become part of the community.
+
+Reviewers can interact with the submissions by: asking questions
+about the code, sharing relevant experience, suggesting changes, etc.
+Asking questions is a particularly useful technique, for example rather
+than stating:
+
+``I think there can be a deadlock between A and B here.``
+
+it may be better to phrase the feedback as a question:
+
+``Could you explain what prevents deadlocks between A and B?``
+
+After all, patch submissions should be clear both in terms of the code and
+the commit message.
+
+Another technique useful in case of a disagreement is to ask for others
+to chime in. I.e. if a discussion reaches a stalemate after a few exchanges,
+calling for opinions of other reviewers or maintainers. Often those in
+agreement with a reviewer remain silent unless called upon.
+Opinion of multiple people carries exponentially more weight.
+
+There is no strict requirement to use specific tags like ``Reviewed-by``.
+In fact reviews in plain English are more informative and encouraged
+even when a tag is provided (e.g. "I looked at aspects A, B and C of this
+submission and it looks good to me.")
+Some form of a review message / reply is obviously necessary otherwise
+maintainers will not know that the reviewer has looked at the patch at all!
+
+It's safe to assume that the maintainers know the community and the level
+of expertise of the reviewers. The reviewers should not be concerned about
+their comments impeding or derailing the patch flow.
+
+Last but not least patch review may become a negative process, focused
+on pointing out problems. Please throw in a complement once in a while,
+particularly for newbies!
+
 Testimonials / feedback
 -----------------------
 
-- 
2.41.0

Re: [RFC] docs: netdev: encourage reviewers

[Edward Cree]

On 06/10/2023 17:30, Jakub Kicinski wrote:
> Add a section to our maintainer doc encouraging reviewers
> to chime in on the mailing list.
> 
> The questions about "when is it okay to share feedback"
> keep coming up (most recently at netconf) and the answer
> is "pretty much always".
> 
> The contents are partially based on a doc we wrote earlier
> and shared with the vendors (for the "driver review rotation").
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
[...]
> +Last but not least patch review may become a negative process, focused
> +on pointing out problems. Please throw in a complement once in a while,
> +particularly for newbies!

sp: compliment.

Otherwise looks good.
-ed

Re: [RFC] docs: netdev: encourage reviewers

[Florian Fainelli]

On 10/6/23 09:30, Jakub Kicinski wrote:
> Add a section to our maintainer doc encouraging reviewers
> to chime in on the mailing list.
> 
> The questions about "when is it okay to share feedback"
> keep coming up (most recently at netconf) and the answer
> is "pretty much always".
> 
> The contents are partially based on a doc we wrote earlier
> and shared with the vendors (for the "driver review rotation").
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

LGTM with the typo that Edward spotted, thanks!
-- 
Florian

Re: [RFC] docs: netdev: encourage reviewers

[Jiri Pirko]

Fri, Oct 06, 2023 at 06:30:07PM CEST, kuba@kernel.org wrote:
>Add a section to our maintainer doc encouraging reviewers
>to chime in on the mailing list.
>
>The questions about "when is it okay to share feedback"
>keep coming up (most recently at netconf) and the answer
>is "pretty much always".
>
>The contents are partially based on a doc we wrote earlier
>and shared with the vendors (for the "driver review rotation").
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Looks awesome. Thanks for doing this!

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [RFC] docs: netdev: encourage reviewers

[Andrew Lunn]

On Fri, Oct 06, 2023 at 09:30:07AM -0700, Jakub Kicinski wrote:
> Add a section to our maintainer doc encouraging reviewers
> to chime in on the mailing list.
> 
> The questions about "when is it okay to share feedback"
> keep coming up (most recently at netconf) and the answer
> is "pretty much always".
> 
> The contents are partially based on a doc we wrote earlier
> and shared with the vendors (for the "driver review rotation").
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> CC: andrew@lunn.ch
> CC: jesse.brandeburg@intel.com
> CC: sd@queasysnail.net
> CC: horms@verge.net.au
> 
> Sending as RFC for early round of reviews before I CC docs@
> and expose this to potentially less constructive feedback :)

We already have:

https://docs.kernel.org/process/7.AdvancedTopics.html#reviewing-patches

which has some of the same concepts. I don't think anything in the
proposed new text is specific to netdev, unlike most of the rest of
maintainer-netdev.rst which does reference netdev specific rules or
concepts.

So i wounder if this even belongs in netdev? Do we actually want to
extend the current text in "A guide to the Kernel Development
Process", and maintainer-netdev.rst say something like:

    Reviewing other people's patches on the list is highly encouraged,
    regardless of the level of expertise.

and cross reference to the text in section 7.2?

    Andrew

Re: [RFC] docs: netdev: encourage reviewers

[Jakub Kicinski]

On Fri, 6 Oct 2023 20:41:19 +0200 Andrew Lunn wrote:
> We already have:
> 
> https://docs.kernel.org/process/7.AdvancedTopics.html#reviewing-patches
> 
> which has some of the same concepts. I don't think anything in the
> proposed new text is specific to netdev, unlike most of the rest of
> maintainer-netdev.rst which does reference netdev specific rules or
> concepts.
> 
> So i wounder if this even belongs in netdev? Do we actually want to
> extend the current text in "A guide to the Kernel Development
> Process", and maintainer-netdev.rst say something like:
> 
>     Reviewing other people's patches on the list is highly encouraged,
>     regardless of the level of expertise.
> 
> and cross reference to the text in section 7.2?

:) If I can't get it past you there's no chance I'll get it past docs@

Let me move some of the staff into general docs and add a reference.
The questions which came up were about use of tags and how maintainers
approach the reviews from less experienced devs, which I think is
subsystem-specific?

Re: [RFC] docs: netdev: encourage reviewers

[Jakub Kicinski]

On Fri, 6 Oct 2023 11:57:15 -0700 Jakub Kicinski wrote:
> :) If I can't get it past you there's no chance I'll get it past docs@
> 
> Let me move some of the staff into general docs and add a reference.
> The questions which came up were about use of tags and how maintainers
> approach the reviews from less experienced devs, which I think is
> subsystem-specific?

So moved most of the paragraphs to the common docs, what I kept in
netdev is this:


Reviewer guidance
-----------------

Reviewing other people's patches on the list is highly encouraged,
regardless of the level of expertise. For general guidance and
helpful tips please see :ref:`development_advancedtopics_reviews`.

It's safe to assume that netdev maintainers know the community and the level
of expertise of the reviewers. The reviewers should not be concerned about
their comments impeding or derailing the patch flow.

Less experienced reviewers should avoid commenting exclusively on more
trivial / subjective matters like code formatting and process aspects
(e.g. missing subject tags).


Sounds reasonable?

Re: [RFC] docs: netdev: encourage reviewers

[Andrew Lunn]

> So moved most of the paragraphs to the common docs, what I kept in
> netdev is this:
> 
> 
> Reviewer guidance
> -----------------
> 
> Reviewing other people's patches on the list is highly encouraged,
> regardless of the level of expertise. For general guidance and
> helpful tips please see :ref:`development_advancedtopics_reviews`.
> 
> It's safe to assume that netdev maintainers know the community and the level
> of expertise of the reviewers. The reviewers should not be concerned about
> their comments impeding or derailing the patch flow.
> 
> Less experienced reviewers should avoid commenting exclusively on more
> trivial / subjective matters like code formatting and process aspects
> (e.g. missing subject tags).
> 
> 
> Sounds reasonable?

Yes, that is reasonable. And i agree that i get the feeling some
subsystems don't know their community members too well, enough to
establish a good idea of trust or not.

Thanks
	Andrew

Re: [RFC] docs: netdev: encourage reviewers

[Florian Fainelli]

On 10/6/23 12:10, Jakub Kicinski wrote:
> On Fri, 6 Oct 2023 11:57:15 -0700 Jakub Kicinski wrote:
>> :) If I can't get it past you there's no chance I'll get it past docs@
>>
>> Let me move some of the staff into general docs and add a reference.
>> The questions which came up were about use of tags and how maintainers
>> approach the reviews from less experienced devs, which I think is
>> subsystem-specific?
> 
> So moved most of the paragraphs to the common docs, what I kept in
> netdev is this:
> 
> 
> Reviewer guidance
> -----------------
> 
> Reviewing other people's patches on the list is highly encouraged,
> regardless of the level of expertise. For general guidance and
> helpful tips please see :ref:`development_advancedtopics_reviews`.
> 
> It's safe to assume that netdev maintainers know the community and the level
> of expertise of the reviewers. The reviewers should not be concerned about
> their comments impeding or derailing the patch flow.
> 
> Less experienced reviewers should avoid commenting exclusively on more
> trivial / subjective matters like code formatting and process aspects
> (e.g. missing subject tags).

Would rephrase the last paragraph and propose:

Less experienced reviewers are highly encouraged to do more in-depth 
review of submissions and not focus exclusively on trivial / subject 
matters like code formatting, tags etc.


-- 
Florian

Re: [RFC] docs: netdev: encourage reviewers

[Przemek Kitszel]

On 10/6/23 21:10, Jakub Kicinski wrote:
> On Fri, 6 Oct 2023 11:57:15 -0700 Jakub Kicinski wrote:
>> :) If I can't get it past you there's no chance I'll get it past docs@
>>
>> Let me move some of the staff into general docs and add a reference.
>> The questions which came up were about use of tags and how maintainers
>> approach the reviews from less experienced devs, which I think is
>> subsystem-specific?
> 
> So moved most of the paragraphs to the common docs, what I kept in
> netdev is this:
> 
> 
> Reviewer guidance
> -----------------
> 
> Reviewing other people's patches on the list is highly encouraged,
> regardless of the level of expertise. For general guidance and
> helpful tips please see :ref:`development_advancedtopics_reviews`.
> 
> It's safe to assume that netdev maintainers know the community and the level
> of expertise of the reviewers. The reviewers should not be concerned about
> their comments impeding or derailing the patch flow.
> 
> Less experienced reviewers should avoid commenting exclusively on more
> trivial / subjective matters like code formatting and process aspects
> (e.g. missing subject tags).

that should be taken for granted for experienced-but-new-to-community
reviewer, but perhaps s/Less experienced/New/?

> 
> 
> Sounds reasonable?

yes!

other thing, somewhat related:
I believe that I personally (new to community, ~new to Intel) would do
"best" for community trying to focus most on our outgoing patches (so
"pre-review" wrt. to netdev, but less traffic here).


anyway,
I feel encouraged here :)

Re: [RFC] docs: netdev: encourage reviewers

[Jakub Kicinski]

On Mon, 9 Oct 2023 17:13:45 +0200 Przemek Kitszel wrote:
> > Less experienced reviewers should avoid commenting exclusively on more
> > trivial / subjective matters like code formatting and process aspects
> > (e.g. missing subject tags).  
> 
> that should be taken for granted for experienced-but-new-to-community
> reviewer, but perhaps s/Less experienced/New/?

The set of Not new + Not experienced is not empty, right?
We want people to be cautious about posting such comments.
IOW what if people have been around for a while but aren't
very well versed in reviewing on netdev?

> > Sounds reasonable?  
> 
> yes!
> 
> other thing, somewhat related:
> I believe that I personally (new to community, ~new to Intel) would do
> "best" for community trying to focus most on our outgoing patches (so
> "pre-review" wrt. to netdev, but less traffic here).

Herm, yes, definitely. It's a separate topic, tho, because the folks
at Intel who will read the FAQ are also those who understand the
problems with the internal process. The problem is not new, to move 
the corporate processes we need metrics. I think.

> anyway,
> I feel encouraged here :)

Perfect :)