Message-ID: <87iktl661o.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Mon, 21 Oct 2024 19:37:07 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Alejandro Lucero Palau <alucerop@....com>,  Dan Williams
 <dan.j.williams@...el.com>,  Dave Jiang <dave.jiang@...el.com>,
  <linux-cxl@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,  Gregory
 Price <gourry@...rry.net>,  Davidlohr Bueso <dave@...olabs.net>,  Alison
 Schofield <alison.schofield@...el.com>,  Vishal Verma
 <vishal.l.verma@...el.com>,  "Ira Weiny" <ira.weiny@...el.com>,  Ben
 Cheatham <benjamin.cheatham@....com>
Subject: Re: [PATCH 5/5] cxl: Avoid to create dax regions for type2
 accelerators

Hi, Jonathan,

Jonathan Cameron <Jonathan.Cameron@...wei.com> writes:

> On Thu, 17 Oct 2024 15:48:04 +0800
> "Huang, Ying" <ying.huang@...el.com> wrote:
>
>> Alejandro Lucero Palau <alucerop@....com> writes:
>> 
>> > On 10/17/24 07:29, Huang, Ying wrote:  
>> >> Hi, Alejandro,
>> >>
>> >> Alejandro Lucero Palau <alucerop@....com> writes:
>> >>  
>> >>> I did comment on this some time ago and I'm doing it again.
>> >>>
>> >>>
>> >>> This is originally part of the type2 patchset, and I'm keeping it in
>> >>> V4. I do not understand why you pick code changes (you explicitly said
>> >>> that in the first RFC) from there and use it here, and without
>> >>> previous discussion about this necessity in the list. I do not think
>> >>> this is usual, at least in other kernel subsystems I'm more familiar
>> >>> with, so I will raise this in today's cxl open source collaboration
>> >>> sync.  
>> >> No.  I picked this change from Dan's series as follows,
>> >>
>> >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html
>> >>
>> >> So, I added co-developed-by and signed-off-by of Dan.
>> >>
>> >> IIUC, your picked this change from Dan's series too?  
>> >
>> >
>> > Look, this is not going well.
>
> Hi Alejandro + Huang, Ying
>
> This seems to be an unfortunate case of disconnected work on the same
> large problem and shows the need for more coordination.
> Note these are my personal responses to this, other maintainers and
> community members may well disagree!
>
> Alejandro had already clearly adopted the patches from Dan and taken a
> number of them forwards as part of his patch set.  That had
> happened before Huang, Ying posted the RFC (which referenced
> Alejandro's work along side Dan's original series).
>
> The idea of trying to accelerate the process of upstreaming type 2
> support by merging a few low hanging fruit is certainly one I think
> we can all get behind.  However, it needs some coordination to avoid
> actually slowing down overall progress by both causing spread out
> reviews and divergence in direction + churn in the base on which
> the fuller sets are built. So Huang, Ying please work with Alejandro
> rather than continuing to evolve this set independently.

IMHO, it may be better to continue to review this small series and make
it ready to be merged (or dropped) ASAP?  I can put it as a high
priority task.  After all, this series begins with code or ideas from
Dan.

However, this is only my personal opinion, if community thinks that it
isn't a good idea, I can drop this series, at least the part that
duplicates with Alejandro's series.

> Perhaps an initial step would be to figure out how to reorder Alejandro's
> series so that any work duplicated by this set is pulled to the front.
> That should make it easy to identify and discuss differences that
> have resulted from review of this series. At that point we can focus
> the review on those patches as the rest of the set continues to evolve.
> However, I would  strongly suggest coordinating that work in order to
> avoid churning the code when Alejandro may be near to posting a new
> version of his fuller series.
> 
> Whilst the precise way we have ended up with two sets changing the same
> code is unusual it is extremely common for multiple people to be working
> on the same code and coordination to be needed.  Many of the regular
> sync calls / discord channels / irc etc are used to figure out how
> this can be done efficiently.  Please use those channels here.

Yes.  More coordination is good.

> If it would be useful to have an additional call or similar to ensure
> a fruitful collaboration then we can set one up.
>
> Finally I'll note that I'd have expected to see explicit discussion of
> how this series relates to Alejandro's set and a suggestion of how to
> move forwards in the cover letter and that would perhaps have either
> resolved Alejandro's concerns or at least publicly shown awareness of the
> issues this would cause for his work.
>
> Irrespective of the other reasons for such an intro text, whilst the
> CXL maintainers were at least somewhat aware, we always appreciate
> a reminder in a cover letter!
>
> Jonathan
>
>
>> >
>> >
>> > You specifically said in your first patchset you considered the type2
>> > support patchset complete but too large or complex, so you were taking
>> > parts of it as a prelude for making it easier to review/accept. Just
>> > face that and not twist the argument.  
>> 
>> Although I listed your patchset in my cover letter.  All changes I
>> picked was from Dan's patchset instead of yours.  And, I kept Dan's
>> co-developed-by and signed-off-by.  If you will pick changes from Dan,
>> please do that too.
>> 
>> > FWIW, I'm against you doing so because:
>> >
>> >
>> > 1) You should have commented in the type2 patchset about your concern,
>> > and gave advice about doing such a prelude (by me) or offer yourself
>> > for doing it.
>> >
>> > 2) Just following your approach, anyone could do the same for any
>> > patchset sent to the list. This is not a good precedent.
>> >
>> > 3) If this is going to be allowed/approved, I'm not going to be
>> > comfortable within this community. If it is just me, I guess it will
>> > not be a big loss.
>> >
>> >
>> > None has commented yet except you and me, what I do not know if it is
>> > because this is a nasty discussion they do not want to get entangle
>> > with, or because they just think your approach is OK. If not further
>> > comment and your patchset is accepted, nothing else will be needed to
>> > say.
>> >
>> >  
>> >> Feel free to include this change in your series.  If your patchset is
>> >> merged firstly, I will rebase on yours and drop this change.
>> >>
>> >> [snip]  
>> 

--
Best Regards,
Huang, Ying

Powered by blists - more mailing lists