[PATCH] docs: maintainer: discourage taking conversations off-list
Multiple vendors seem to prefer taking discussions off list, and ask contributors to work with them privately rather than just send patches to the list. I'd imagine this is because it's hard to fit in time for random developers popping up with features to review into packed schedule. From what I've seen "work in private" usually means someone on the company side will be assigned to handle the interaction, possibly months later. In worst case, the person scheduled to help the contributor takes over and writes the code themselves. This is not how the community is supposed to work. Signed-off-by: Jakub Kicinski --- CC: workfl...@vger.kernel.org CC: linux-doc@vger.kernel.org --- .../maintainer/feature-and-driver-maintainers.rst | 11 +++ 1 file changed, 11 insertions(+) diff --git a/Documentation/maintainer/feature-and-driver-maintainers.rst b/Documentation/maintainer/feature-and-driver-maintainers.rst index f04cc183e1de..ac7798280201 100644 --- a/Documentation/maintainer/feature-and-driver-maintainers.rst +++ b/Documentation/maintainer/feature-and-driver-maintainers.rst @@ -83,6 +83,17 @@ bugs as well, if the report is of reasonable quality or indicates a problem that might be severe -- especially if they have *Supported* status of the codebase in the MAINTAINERS file. +Open development + + +Discussions about user reported issues, and development of new code +should be conducted in a manner typical for the larger subsystem. +It is common for development within a single company to be conducted +behind closed doors. However, maintainers must not redirect discussions +and development related to the upstream code from the upstream mailing lists +to closed forums or private conversations. Reasonable exceptions to this +guidance include discussions about security related issues. + Selecting the maintainer -- 2.45.2
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On Fri, Jul 12, 2024 at 07:49:03AM -0700, Jakub Kicinski wrote: > Multiple vendors seem to prefer taking discussions off list, and > ask contributors to work with them privately rather than just send > patches to the list. I'd imagine this is because it's hard to fit in > time for random developers popping up with features to review into > packed schedule. From what I've seen "work in private" usually means > someone on the company side will be assigned to handle the interaction, > possibly months later. In worst case, the person scheduled to help > the contributor takes over and writes the code themselves. > This is not how the community is supposed to work. > > Signed-off-by: Jakub Kicinski > --- > CC: workfl...@vger.kernel.org > CC: linux-doc@vger.kernel.org > --- > .../maintainer/feature-and-driver-maintainers.rst | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/maintainer/feature-and-driver-maintainers.rst > b/Documentation/maintainer/feature-and-driver-maintainers.rst > index f04cc183e1de..ac7798280201 100644 > --- a/Documentation/maintainer/feature-and-driver-maintainers.rst > +++ b/Documentation/maintainer/feature-and-driver-maintainers.rst > @@ -83,6 +83,17 @@ bugs as well, if the report is of reasonable quality or > indicates a > problem that might be severe -- especially if they have *Supported* > status of the codebase in the MAINTAINERS file. > > +Open development > + > + > +Discussions about user reported issues, and development of new code > +should be conducted in a manner typical for the larger subsystem. > +It is common for development within a single company to be conducted > +behind closed doors. However, maintainers must not redirect discussions > +and development related to the upstream code from the upstream mailing lists > +to closed forums or private conversations. Reasonable exceptions to this > +guidance include discussions about security related issues. > + > Selecting the maintainer > > Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On Fri, Jul 12, 2024 at 07:49:03AM -0700, Jakub Kicinski wrote: > +Open development > + > + > +Discussions about user reported issues, and development of new code > +should be conducted in a manner typical for the larger subsystem. > +It is common for development within a single company to be conducted > +behind closed doors. However, maintainers must not redirect discussions > +and development related to the upstream code from the upstream mailing lists > +to closed forums or private conversations. Reasonable exceptions to this > +guidance include discussions about security related issues. Reviewed-by: Mark Brown I do think it's fine for people to have open places like github as an option as well, so long as they're optional for contributors and things pass through the lists in a normal fashion at some point. Directing people towards existing relevant discussions/reviews can work well. The main issues are taking things out of all visibility and blocking contributors. signature.asc Description: PGP signature
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On 7/12/24 09:25, Mark Brown wrote: On Fri, Jul 12, 2024 at 07:49:03AM -0700, Jakub Kicinski wrote: +Open development + + +Discussions about user reported issues, and development of new code +should be conducted in a manner typical for the larger subsystem. +It is common for development within a single company to be conducted +behind closed doors. However, maintainers must not redirect discussions +and development related to the upstream code from the upstream mailing lists +to closed forums or private conversations. Reasonable exceptions to this +guidance include discussions about security related issues. Reviewed-by: Mark Brown I do think it's fine for people to have open places like github as an option as well, so long as they're optional for contributors and things pass through the lists in a normal fashion at some point. Directing people towards existing relevant discussions/reviews can work well. The main issues are taking things out of all visibility and blocking contributors. +1 It is important to point out the prior open conversations if any that took place when patches go through reviews on kernel mailing lists. This is the practice for the most part for work stemming from discussions at conferences such as LPC. Reviewed-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 11/13] huge_memory: Remove dead vmf_insert_pXd code
On Fri, Jul 12, 2024 at 12:40:39PM +1000, Alistair Popple wrote:
>
> Peter Xu writes:
>
> > On Tue, Jul 09, 2024 at 02:07:31PM +1000, Alistair Popple wrote:
> >>
> >> Peter Xu writes:
> >>
> >> > Hi, Alistair,
> >> >
> >> > On Thu, Jun 27, 2024 at 10:54:26AM +1000, Alistair Popple wrote:
> >> >> Now that DAX is managing page reference counts the same as normal
> >> >> pages there are no callers for vmf_insert_pXd functions so remove
> >> >> them.
> >> >>
> >> >> Signed-off-by: Alistair Popple
> >> >> ---
> >> >> include/linux/huge_mm.h | 2 +-
> >> >> mm/huge_memory.c| 165
> >> >> +-
> >> >> 2 files changed, 167 deletions(-)
> >> >>
> >> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> >> index 9207d8e..0fb6bff 100644
> >> >> --- a/include/linux/huge_mm.h
> >> >> +++ b/include/linux/huge_mm.h
> >> >> @@ -37,8 +37,6 @@ int change_huge_pmd(struct mmu_gather *tlb, struct
> >> >> vm_area_struct *vma,
> >> >> pmd_t *pmd, unsigned long addr, pgprot_t newprot,
> >> >> unsigned long cp_flags);
> >> >>
> >> >> -vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool
> >> >> write);
> >> >> -vm_fault_t vmf_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool
> >> >> write);
> >> >> vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool
> >> >> write);
> >> >> vm_fault_t dax_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool
> >> >> write);
> >> >
> >> > There's a plan to support huge pfnmaps in VFIO, which may still make good
> >> > use of these functions. I think it's fine to remove them but it may mean
> >> > we'll need to add them back when supporting pfnmaps with no memmap.
> >>
> >> I'm ok with that. If we need them back in future it shouldn't be too
> >> hard to add them back again. I just couldn't find any callers of them
> >> once DAX stopped using them and the usual policy is to remove unused
> >> functions.
> >
> > True. Currently the pmd/pud helpers are only used in dax.
> >
> >>
> >> > Is it still possible to make the old API generic to both service the new
> >> > dax refcount plan, but at the meantime working for pfn injections when
> >> > there's no page struct?
> >>
> >> I don't think so - this new dax refcount plan relies on having a struct
> >> page to take references on so I don't think it makes much sense to
> >> combine it with something that doesn't have a struct page. It sounds
> >> like the situation is the analogue of vm_insert_page()
> >> vs. vmf_insert_pfn() - it's possible for both to exist but there's not
> >> really anything that can be shared between the two APIs as one has a
> >> page and the other is just a raw PFN.
> >
> > I still think most of the codes should be shared on e.g. most of sanity
> > checks, pgtable injections, pgtable deposits (for pmd) and so on.
>
> Yeah, it was mostly the BUG_ON's that weren't applicable once pXd_devmap
> went away.
>
> > To be explicit, I wonder whether something like below diff would be
> > applicable on top of the patch "huge_memory: Allow mappings of PMD sized
> > pages" in this series, which introduced dax_insert_pfn_pmd() for dax:
> >
> > $ diff origin new
> > 1c1
> > < vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
> > ---
> >> vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
> > 55,58c55,60
> > < folio = page_folio(page);
> > < folio_get(folio);
> > < folio_add_file_rmap_pmd(folio, page, vma);
> > < add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
> > ---
> >> if (page) {
> >> folio = page_folio(page);
> >> folio_get(folio);
> >> folio_add_file_rmap_pmd(folio, page, vma);
> >> add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
> >> }
>
> We get the page from calling pfn_t_to_page(pfn). This is safe for the
> DAX case but is it safe to use a page returned by this more generally?
Good question. I thought it should work when the caller doesn't set any
bit in PFN_FLAGS_MASK, but it turns out it's not the case? As I just
notice:
static inline bool pfn_t_has_page(pfn_t pfn)
{
return (pfn.val & PFN_MAP) == PFN_MAP || (pfn.val & PFN_DEV) == 0;
}
So it looks like "no PFN_FLAGS" case should also fall into this category of
"(pfn.val & PFN_DEV) == 0"..
I'm not sure whether my understanding is correct, though. Maybe we'd want
to double check with pfn_valid() when it's a generic function.
>
> From an API perspective it would make more sense for the DAX code to
> pass the page rather than the pfn. I didn't do that because device DAX
> just had the PFN and this was DAX-specific code. But if we want to make
> it generic I'd rather have callers pass the page in.
>
> Of course that probably doesn't help you, because then the call would be
> vmf_insert_page_pmd() rather than a raw pfn, but as you point out t
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
Em Fri, 12 Jul 2024 09:42:07 -0600 Shuah Khan escreveu: > On 7/12/24 09:25, Mark Brown wrote: > > On Fri, Jul 12, 2024 at 07:49:03AM -0700, Jakub Kicinski wrote: > > > >> +Open development > >> + > >> + > >> +Discussions about user reported issues, and development of new code > >> +should be conducted in a manner typical for the larger subsystem. > >> +It is common for development within a single company to be conducted > >> +behind closed doors. True. So what? > >> However, maintainers must not redirect discussions > >> +and development related to the upstream code from the upstream mailing > >> lists > >> +to closed forums or private conversations. Reasonable exceptions to this > >> +guidance include discussions about security related issues. Not sure what this somewhat obscure message wants to accomplish. It is quite common to have developers and maintainers discussing outside public forums and internally at the companies they're working for. There are lots of reasonable exceptions besides security. On my years of experience, the reasons I've seen more often are: 1. language and/or cultural barriers; 2. teaching and mentoring new developers to start contributing upstream; 3. need to have internal discussions in the light of some IP protected material. (1) and (2) are very common for non-native English speakers and for newbies, and we do want to have more contributions from them. (3) is unavoidable, as discussions related to protected IP can't be disclosed due to legal reasons. Also, if you take it to the letter, have discussions on LPC, summits BoFs, other events handled by the open source community and wall conversations are "closed forums/private conversations". I've seen a lot of good results produced on such private conversations that solved relevant conflicts and got materialized as great contributions. There's nothing wrong with that, provided that the outcoming of such internal discussions are reflected publicly as patches, summit minutes, LWN articles, etc. The only issues I see with such talks is that the work when co-authored should be properly marked as such and that reviewews/acks taken behind doors don't have the same meaning as an upstream review, as they may be due to some internal formalities. IMO, the best would instead to give a positive message. E. g. something like: Maintainers must encourage discussions and reviews to happen at public mailing lists, avoiding whenever possible to have internal discussions. Regards, Mauro
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On 7/12/24 11:11 AM, Mauro Carvalho Chehab wrote: > IMO, the best would instead to give a positive message. E. g. > something like: > > Maintainers must encourage discussions and reviews to happen > at public mailing lists, avoiding whenever possible to have > internal discussions. Ack that. with s/at public/on public/ -- ~Randy
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
Jakub Kicinski wrote: > Multiple vendors seem to prefer taking discussions off list, and > ask contributors to work with them privately rather than just send > patches to the list. I'd imagine this is because it's hard to fit in > time for random developers popping up with features to review into > packed schedule. From what I've seen "work in private" usually means > someone on the company side will be assigned to handle the interaction, > possibly months later. In worst case, the person scheduled to help > the contributor takes over and writes the code themselves. > This is not how the community is supposed to work. > > Signed-off-by: Jakub Kicinski > --- > CC: workfl...@vger.kernel.org > CC: linux-doc@vger.kernel.org > --- > .../maintainer/feature-and-driver-maintainers.rst | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/maintainer/feature-and-driver-maintainers.rst > b/Documentation/maintainer/feature-and-driver-maintainers.rst > index f04cc183e1de..ac7798280201 100644 > --- a/Documentation/maintainer/feature-and-driver-maintainers.rst > +++ b/Documentation/maintainer/feature-and-driver-maintainers.rst > @@ -83,6 +83,17 @@ bugs as well, if the report is of reasonable quality or > indicates a > problem that might be severe -- especially if they have *Supported* > status of the codebase in the MAINTAINERS file. > > +Open development > + > + > +Discussions about user reported issues, and development of new code > +should be conducted in a manner typical for the larger subsystem. > +It is common for development within a single company to be conducted > +behind closed doors. However, maintainers must not redirect discussions > +and development related to the upstream code from the upstream mailing lists > +to closed forums or private conversations. Reasonable exceptions to this > +guidance include discussions about security related issues. > + This reads as a vague ambiguous quasi-threat with no actionable way to enforce it. In contrast, successful maintainers already have a sense of the benefits of pushing discussions to the list as much as possible. For new and growing maintainers, which I assume are the audience for this guidance, that are unaware of the pitfalls of taking conversations off-list, they likely need help understanding the *benefits* of open development. So if this goes in as is, so be it, but it feels like a missed opportunity to extoll the virtues of open development. The benefits are in the same vector as the "release early, release often" guidance where the urge to polish a solution in private is a common trait of newcomers. Lets document some tribal knowledge of how one moves past that first instinct.
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On Fri, 12 Jul 2024 20:11:56 +0200 Mauro Carvalho Chehab wrote: > Not sure what this somewhat obscure message wants to accomplish. > > It is quite common to have developers and maintainers discussing > outside public forums and internally at the companies they're working > for. There are lots of reasonable exceptions besides security. On my > years of experience, the reasons I've seen more often are: > > 1. language and/or cultural barriers; > 2. teaching and mentoring new developers to start contributing upstream; > 3. need to have internal discussions in the light of some IP protected >material. > > (1) and (2) are very common for non-native English speakers > and for newbies, and we do want to have more contributions from > them. (3) is unavoidable, as discussions related to protected > IP can't be disclosed due to legal reasons. Those are valid points but I don't know how to weave them in without losing clarity. The goal is also to call out such behavior to _contributors_, so that they know they can reach out to more senior maintainers if it happens to them. We don't know when discussion is taken private (by definition). Otherwise contributors get mistreated by some corpo-maintainer and they turn away from Linux :( > Also, if you take it to the letter, have discussions on LPC, > summits BoFs, other events handled by the open source community > and wall conversations are "closed forums/private conversations". > I've seen a lot of good results produced on such private > conversations that solved relevant conflicts and got > materialized as great contributions. > > There's nothing wrong with that, provided that the outcoming of > such internal discussions are reflected publicly as patches, > summit minutes, LWN articles, etc. Would it help if we speak of "open forums" instead of mailing list? I think LPC including "hallway track" fall squarely under "conducted in a manner typical for the larger subsystem." Here's slightly edited version: Open development Discussions about user reported issues, and development of new code should be conducted in a manner typical for the larger subsystem. It is common for development within a single company to be conducted behind closed doors. However, development and discussions initiated by community members must not be redirected from public to closed forums or to private email conversations. Reasonable exceptions to this guidance include discussions about security related issues. > The only issues I see with such talks is that the work when > co-authored should be properly marked as such and that > reviewews/acks taken behind doors don't have the same meaning > as an upstream review, as they may be due to some internal > formalities. > > IMO, the best would instead to give a positive message. E. g. > something like: > > Maintainers must encourage discussions and reviews to happen > at public mailing lists, avoiding whenever possible to have > internal discussions. That's not the message, tho. If someone emails a company privately that's fine. If company has internal processes for its development - also fine (as explicitly called out). I'm trying to set the baseline, not describe the ideal world. I am specifically calling out that if someone submits a patch, or reports a regression the correct response is to review it on the list. Like a normal person. Not reply privately that "it's on the company roadmap, just wait" :| Or reply with a patch company has "forgotten to upstream".. Maybe it's a cultural thing, but to me this is where the relentless positivity is counter-productive. I don't want to encourage people to be angles. I want them not to do the shitty thing.
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
Jakub Kicinski wrote:
[..]
> Would it help if we speak of "open forums" instead of mailing list?
> I think LPC including "hallway track" fall squarely under "conducted
> in a manner typical for the larger subsystem." Here's slightly edited
> version:
>
> Open development
>
>
> Discussions about user reported issues, and development of new code
> should be conducted in a manner typical for the larger subsystem.
> It is common for development within a single company to be conducted
> behind closed doors. However, development and discussions initiated
> by community members must not be redirected from public to closed forums
> or to private email conversations. Reasonable exceptions to this guidance
> include discussions about security related issues.
>
> > The only issues I see with such talks is that the work when
> > co-authored should be properly marked as such and that
> > reviewews/acks taken behind doors don't have the same meaning
> > as an upstream review, as they may be due to some internal
> > formalities.
> >
> > IMO, the best would instead to give a positive message. E. g.
> > something like:
> >
> > Maintainers must encourage discussions and reviews to happen
> > at public mailing lists, avoiding whenever possible to have
> > internal discussions.
>
> That's not the message, tho. If someone emails a company privately
> that's fine. If company has internal processes for its development -
> also fine (as explicitly called out). I'm trying to set the baseline,
> not describe the ideal world.
>
> I am specifically calling out that if someone submits a patch, or
> reports a regression the correct response is to review it on the list.
> Like a normal person.
> Not reply privately that "it's on the company roadmap, just wait" :|
> Or reply with a patch company has "forgotten to upstream"..
>
> Maybe it's a cultural thing, but to me this is where the relentless
> positivity is counter-productive. I don't want to encourage people
> to be angles. I want them not to do the shitty thing.
>
To be honest I am lost trying to understand who the audience is and what
the actionable takeaway is from the guidance. It sounds like you are
trying to educate drive-by submitters to push back against requests to
take issues off the list. I think that's a reasonable education
campaign, but doesn't that kind of "submitter bill-of-rights" note
belong in Documentation/admin-guide/reporting-{issues,regressions}.rst
as explicit "how to work your issue upstream" guidance?
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On Fri, 12 Jul 2024 11:43:14 -0700 Dan Williams wrote: > This reads as a vague ambiguous quasi-threat with no actionable way to > enforce it. In contrast, successful maintainers already have a sense of > the benefits of pushing discussions to the list as much as possible. > > For new and growing maintainers, which I assume are the audience for > this guidance, that are unaware of the pitfalls of taking conversations > off-list, they likely need help understanding the *benefits* of open > development. I don't think so. If your boss comes to you and says "Dan, from now on try not to reply to customers on the list, open a ticket first and only reply once tickets is resolved". Is it more useful to you to be able to extol the benefits of open source process; or to tell them "this is not allowed, here's a link to a quasi-threat"? > So if this goes in as is, so be it, but it feels like a missed > opportunity to extoll the virtues of open development. The benefits are > in the same vector as the "release early, release often" guidance where > the urge to polish a solution in private is a common trait of newcomers. > Lets document some tribal knowledge of how one moves past that first > instinct. Hm, the disconnect may be that you think this happens with maintainers without upstream experience. So we can train them up to be better. In most cases it happens to folks with experience who are good maintainers. They just get 2 orders of magnitudes more patches from inside the company that outside. Then a contribution comes from outside, the maintainer is overworked, and tries to shoehorn the patch into the existing, internal-only process.
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
On Fri, 12 Jul 2024 17:00:44 -0700 Dan Williams wrote:
> To be honest I am lost trying to understand who the audience is and what
> the actionable takeaway is from the guidance. It sounds like you are
> trying to educate drive-by submitters to push back against requests to
> take issues off the list. I think that's a reasonable education
> campaign, but doesn't that kind of "submitter bill-of-rights" note
> belong in Documentation/admin-guide/reporting-{issues,regressions}.rst
> as explicit "how to work your issue upstream" guidance?
Fair point. Not sure if reporting-* is a good place, I care about code
contributions 10x more than issues and other discussions. No strong
preference on the placement, as long as its documented...
Re: [PATCH] docs: maintainer: discourage taking conversations off-list
Jakub Kicinski wrote: [..] > > So if this goes in as is, so be it, but it feels like a missed > > opportunity to extoll the virtues of open development. The benefits are > > in the same vector as the "release early, release often" guidance where > > the urge to polish a solution in private is a common trait of newcomers. > > Lets document some tribal knowledge of how one moves past that first > > instinct. > > Hm, the disconnect may be that you think this happens with maintainers > without upstream experience. So we can train them up to be better. > In most cases it happens to folks with experience who are good > maintainers. They just get 2 orders of magnitudes more patches from > inside the company that outside. Then a contribution comes from outside, > the maintainer is overworked, and tries to shoehorn the patch into the > existing, internal-only process. Oh, ok, I was failing to grok that from "Open development" note in isolation. If the guidance is for maintainers, I would say just put your changelog directly into the docuementation, that reads as specific and actionable to me. For submitters an update to reporting-* might be also be useful to remind them to push back on requests to go off-list, but not necessary.
