All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lars Kurth <lars.kurth.xen@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	Wei Liu <wei.liu2@citrix.com>, He Chen <he.chen@linux.intel.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dario Faggioli <dario.faggioli@citrix.com>,
	ian.jackson@eu.citrix.com, Yi Sun <yi.y.sun@linux.intel.com>,
	mengxu@cis.upenn.edu, xen-devel <xen-devel@lists.xenproject.org>,
	chao.p.peng@linux.intel.com,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v10 09/25] x86: refactor psr: L3 CAT: set value: implement framework.
Date: Thu, 20 Apr 2017 17:52:47 +0100	[thread overview]
Message-ID: <862CDEDB-CBD3-4863-8318-4B6902676780@gmail.com> (raw)
In-Reply-To: <58F8D1E4020000780015270E@prv-mh.provo.novell.com>


> On 20 Apr 2017, at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
> 
>>>> On 20.04.17 at 15:02, <lars.kurth.xen@gmail.com> wrote:
>>> On 20 Apr 2017, at 10:43, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>>>>> On 20.04.17 at 04:14, <yi.y.sun@linux.intel.com> wrote:
>>>> On 17-04-19 03:00:29, Jan Beulich wrote:
>>>>>>>> On 19.04.17 at 10:22, <yi.y.sun@linux.intel.com> wrote:
>>>>>> On 17-04-18 05:46:43, Jan Beulich wrote:
>>>>>>>>>> On 18.04.17 at 12:55, <yi.y.sun@linux.intel.com> wrote:
>>>>>>>> I made a test patch based on v10 and attached it in mail. Could you please
>>>>>>>> help to check it? Thanks!
>>>>>>> 

[Item 1]

>>>>>>> This looks reasonable at the first glance, albeit I continue to be
>>>>>>> unconvinced that this is the only (reasonable) way of solving the
>>>> 
>>>> Do you have any other suggestion on this? Thanks!
>>> 
>>> I'm sorry, but no. I'm already spending _much_ more time on this
>>> series than I should be, considering its (afaic) relatively low
>>> priority. I really have to ask you to properly think through both
>>> the data layout and code structure, including considering
>>> alternatives especially in places where you can _anticipate_
>>> controversy.
>> 
>> Jan, can you confirm whether you are happy with the current proposal. You 
>> say "this looks reasonable at the first glance", but then go on to say that 
>> there may be other "reasonable ways" of solving the problem at hand.
>> 
>> This is not very concrete and hard to respond to from a contributors point 
>> of view: it would be good to establish whether a) the v10 proposal in this 
>> area is good enough, b) whether there are any concrete improvements to be 
>> made.

> I understand it's not very concrete, but please understand that with
> the over 100 patches wanting looking at right now it is simply
> impossible for me to give precise suggestions everywhere. I really
> have to be allowed to defer to the originator to come up with
> possible better mechanisms (or defend what there is by addressing
> questions raised),

Jan, I don't object to the principle of deferring issues to a contributor, for contributor to defend their viewpoint or to come up with a better mechanism. I just observed, that I could not make a lot of sense what you were looking for in this particular review. I am assuming that it would be even harder for Yi. 

> especially with - as said - the amount of time spent
> here already having been way higher than is justifiable.

And of course we have passed the 4.9 code freeze, so some of the pressure is off. At the same time I understand that because of the upcoming releases you need to focus on bug fixes, etc.

> Just go and
> compare v10 with one of the initial versions: Almost all of the data
> layout and code flow have fundamentally changed, mostly based on
> feedback I gave. I'm sorry for saying that, but to me this is an
> indication that things hadn't been thought through well in the design
> phase, i.e. before even submitting a first RFC.

That is good feedback which may contain some valuable lessons. Once we are through this (or maybe at the summit) it may be worthwhile to look at what has gone wrong and see how we can do better in future.

[Item 2]

>> You say please think through whether "you can _anticipate_ controversy", but 
>> at the same time you can't currently identify/think of any issues. That seems 
>> to suggest to me that maybe the proposal is good enough. Or that something is 
>> missing, which has not been articulated. Taking into account language 
>> barriers, more clarity would probably be helpful.
> 
> I've given criteria by which I have the gut feeling (but no more)
> that this isn't the right approach. I'm absolutely fine to be
> convinced that my gut feeling is wrong. That would require to
> simply answer the question I raised multiple times, and which was
> repeatedly "answered" by simply re-stating previously expressed
> facts.

I have not followed the full thread. But it seems that we have communications issue there. Normally this happens when expectations don't quite match and one party (in this case Yi) does not quite get what the other one is looking for. Maybe the best approach would be for Yi to get some of these things resolved during a short IRC conversation with you. I did see him and others resolve some previous issues more effectively on IRC. 

> If this reaction of mine is not acceptable, all I can do is refrain
> from further looking at this series. And Yi, I certainly apologize
> for perhaps not doing these reviews wholeheartedly, since -
> as also expressed before - I continue to not really view this as
> very important functionality.

As I said earlier, I stepped in, as I didn't really understand what was going on. I think this is a little clearer now. 

So to summarise: 

On item 1: it appears that you are looking for a some more justification why some of the changes were made, maybe with a rationale for some of the choices that were made. Given that this is quite a complex series which has diverged quite a lot from the design, the goal is to make it easier for either you (or someone else) to sanity check the proposal which on the face of things look OK. But you have some doubts and you can't easily check against the design as it is out-of-date.

On item 2: you think something may not be quite right, but you can't really decide until a couple of questions (not quite sure which, but I am sure Yi can locate them) are answered.

Let me know whether this is actually true. 

> Yet considering for how long some
> of the versions hadn't been looked at by anyone at all, the
> alternative would have been to simply let it sit further without
> looking at it. I actually take this lack of interest by others as an
> indication that I'm not the only one considering this nice to have,
> but no more.

That's a good point. There have been some comments from others, but it is also true that 66% of comments on this series did come from you (see https://xen.biterg.io:443/goto/72a753919aa8e3205d03da7b430d2696). On the other hand, this is also a series which is hard to hand over to someone else.

Regards
Lars



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

  reply	other threads:[~2017-04-20 16:52 UTC|newest]

Thread overview: 114+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-01 13:53 [PATCH v10 00/25] Enable L2 Cache Allocation Technology & Refactor psr.c Yi Sun
2017-04-01 13:53 ` [PATCH v10 01/25] docs: create Cache Allocation Technology (CAT) and Code and Data Prioritization (CDP) feature document Yi Sun
2017-04-01 13:53 ` [PATCH v10 02/25] x86: refactor psr: remove L3 CAT/CDP codes Yi Sun
2017-04-01 13:53 ` [PATCH v10 03/25] x86: refactor psr: implement main data structures Yi Sun
2017-04-03 15:50   ` Jan Beulich
2017-04-05  3:12     ` Yi Sun
2017-04-05  8:20       ` Jan Beulich
2017-04-05  8:45         ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 04/25] x86: move cpuid_count_leaf from cpuid.c to processor.h Yi Sun
2017-04-01 13:53 ` [PATCH v10 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow Yi Sun
2017-04-05 15:10   ` Jan Beulich
2017-04-06  5:49     ` Yi Sun
2017-04-06  8:32       ` Jan Beulich
2017-04-06  9:22         ` Yi Sun
2017-04-06  9:34           ` Jan Beulich
2017-04-06 10:02             ` Yi Sun
2017-04-06 14:02               ` Jan Beulich
2017-04-07  5:17                 ` Yi Sun
2017-04-07  8:48                   ` Jan Beulich
2017-04-07  9:08                     ` Yi Sun
2017-04-07  9:46                       ` Jan Beulich
2017-04-10  3:27                         ` Yi Sun
2017-04-10 12:43                           ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 06/25] x86: refactor psr: L3 CAT: implement Domain init/free and schedule flows Yi Sun
2017-04-05 15:23   ` Jan Beulich
2017-04-06  6:01     ` Yi Sun
2017-04-06  8:34       ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 07/25] x86: refactor psr: L3 CAT: implement get hw info flow Yi Sun
2017-04-05 15:37   ` Jan Beulich
2017-04-06  6:05     ` Yi Sun
2017-04-06  8:36       ` Jan Beulich
2017-04-06 11:16         ` Yi Sun
2017-04-06 14:04           ` Jan Beulich
2017-04-07  5:39             ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 08/25] x86: refactor psr: L3 CAT: implement get value flow Yi Sun
2017-04-05 15:51   ` Jan Beulich
2017-04-06  6:10     ` Yi Sun
2017-04-06  8:40       ` Jan Beulich
2017-04-06 11:13         ` Yi Sun
2017-04-06 14:08           ` Jan Beulich
2017-04-07  5:40             ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 09/25] x86: refactor psr: L3 CAT: set value: implement framework Yi Sun
2017-04-11 15:01   ` Jan Beulich
2017-04-12  5:53     ` Yi Sun
2017-04-12  9:09       ` Jan Beulich
2017-04-12 12:23         ` Yi Sun
2017-04-12 12:42           ` Jan Beulich
2017-04-13  8:11             ` Yi Sun
2017-04-13  9:41               ` Jan Beulich
2017-04-13 10:49                 ` Yi Sun
2017-04-13 10:58                   ` Jan Beulich
2017-04-13 11:11                     ` Yi Sun
2017-04-13 11:26                       ` Yi Sun
2017-04-13 11:31                       ` Jan Beulich
2017-04-13 11:44                         ` Yi Sun
2017-04-13 11:50                           ` Jan Beulich
2017-04-18 10:55                             ` Yi Sun
2017-04-18 11:46                               ` Jan Beulich
2017-04-19  8:22                                 ` Yi Sun
2017-04-19  9:00                                   ` Jan Beulich
2017-04-20  2:14                                     ` Yi Sun
2017-04-20  9:43                                       ` Jan Beulich
2017-04-20 13:02                                         ` Lars Kurth
2017-04-20 13:21                                           ` Jan Beulich
2017-04-20 16:52                                             ` Lars Kurth [this message]
2017-04-21  6:11                                               ` Jan Beulich
2017-04-21  1:13                                             ` Konrad Rzeszutek Wilk
2017-04-21  6:18                                       ` Jan Beulich
2017-04-24  6:40                                         ` Yi Sun
2017-04-24  6:55                                           ` Jan Beulich
2017-04-25  7:15                                             ` Yi Sun
2017-04-25  8:24                                               ` Jan Beulich
2017-04-25  8:40                                                 ` Yi Sun
2017-04-20  5:38   ` [PATCH] dom_ids array implementation Yi Sun
2017-04-26 10:04     ` Jan Beulich
2017-04-27  2:38       ` Yi Sun
2017-04-27  6:48         ` Jan Beulich
2017-04-27  9:30           ` Yi Sun
2017-04-27  9:39             ` Jan Beulich
2017-04-27 12:03               ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 10/25] x86: refactor psr: L3 CAT: set value: assemble features value array Yi Sun
2017-04-11 15:11   ` Jan Beulich
2017-04-12  5:55     ` Yi Sun
2017-04-12  9:13       ` Jan Beulich
2017-04-12 12:26         ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 11/25] x86: refactor psr: L3 CAT: set value: implement cos finding flow Yi Sun
2017-04-11 15:17   ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow Yi Sun
2017-04-11 15:20   ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 13/25] x86: refactor psr: L3 CAT: set value: implement write msr flow Yi Sun
2017-04-11 15:25   ` Jan Beulich
2017-04-12  6:04     ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 14/25] x86: refactor psr: CDP: implement CPU init and free flow Yi Sun
2017-04-01 13:53 ` [PATCH v10 15/25] x86: refactor psr: CDP: implement get hw info flow Yi Sun
2017-04-01 13:53 ` [PATCH v10 16/25] x86: refactor psr: CDP: implement get value flow Yi Sun
2017-04-11 15:39   ` Jan Beulich
2017-04-12  6:05     ` Yi Sun
2017-04-12  9:14       ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 17/25] x86: refactor psr: CDP: implement set value callback functions Yi Sun
2017-04-11 16:03   ` Jan Beulich
2017-04-12  6:14     ` Yi Sun
2017-04-01 13:53 ` [PATCH v10 18/25] x86: L2 CAT: implement CPU init and free flow Yi Sun
2017-04-12 15:18   ` Jan Beulich
2017-04-13  8:12     ` Yi Sun
2017-04-13  8:16       ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 19/25] x86: L2 CAT: implement get hw info flow Yi Sun
2017-04-01 13:53 ` [PATCH v10 20/25] x86: L2 CAT: implement get value flow Yi Sun
2017-04-01 13:53 ` [PATCH v10 21/25] x86: L2 CAT: implement set " Yi Sun
2017-04-12 15:23   ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 22/25] tools: L2 CAT: support get HW info for L2 CAT Yi Sun
2017-04-12 15:24   ` Jan Beulich
2017-04-01 13:53 ` [PATCH v10 23/25] tools: L2 CAT: support show cbm " Yi Sun
2017-04-01 13:53 ` [PATCH v10 24/25] tools: L2 CAT: support set " Yi Sun
2017-04-01 13:53 ` [PATCH v10 25/25] docs: add L2 CAT description in docs Yi Sun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=862CDEDB-CBD3-4863-8318-4B6902676780@gmail.com \
    --to=lars.kurth.xen@gmail.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=chao.p.peng@linux.intel.com \
    --cc=dario.faggioli@citrix.com \
    --cc=he.chen@linux.intel.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jbeulich@suse.com \
    --cc=kevin.tian@intel.com \
    --cc=mengxu@cis.upenn.edu \
    --cc=roger.pau@citrix.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    --cc=yi.y.sun@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.