[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v6][PATCH 10/16] tools: introduce some new parameters to set rdm policy



On 2015/7/8 19:47, Ian Jackson wrote:
Tiejun Chen writes ("[v6][PATCH 10/16] tools: introduce some new parameters to set 
rdm policy"):
This patch introduces user configurable parameters to specify RDM
resource and according policies,

Thanks.


I appreciate that I have come to this review late.  While I have found
the review conversation quite unsatisfactory, I don't really feel that
I can reject the patch series pending better answers to my questions.

Instead, I feel that I need to make a set of decisions which will
avoid my review comments being a blocker for this series.  After
discussing matters with the other tools maintainers, I have concluded:


* On the question of whether the default should be `strategy=host' or
   `strategy=none':

   I still don't understand what is going on here and I am frustrated
   because I don't feel that the replies I have been getting are
   actually answers to my questions.  They seem to be answers to
   different questions.

   However, the patch series with `strategy=none' is strictly less of a
   change to the codebase than with `stategy=host' and it is easy to
   change defaults later.  It would be perverse to block this
   functionality on the grounds that it is not enabled strongly enough
   by default.

   Therefore, despite the fact that after several rounds of emails I
   still do not have a convincing explanation, I am going to drop this
   line of questioning.

Maybe I can't answer everything as you expected.

The most important reason why we set 'strategy=none' is that, the RDM issue rarely happens in real world, and especially there are four conditions to trigger this issue.

#1. RDM should exist on this platform

#2. The associated device is passed through

Note so far, just IGD needs this RDM in the real world as we know.

#3. RDM regions should overlap normal RAM in a VM

Its not always true.

#4. The associated device is really accessing RDM.

Just Windows IGD driver is trying to access this. Instead, Linux IGD driver never walk into this.

So we don't like to setting as "host" to concern this sort of small probability event.



* On the question of the documentation: The documentation is
   unfortunately a poor guide to a user.  Many of my questions were
   prompted by reading the documentation.  Having gone several rounds
   of emails I still do not know enough to suggest improvements.

   In my view the effect of the poor documentation will be that most
   users will simply ignore the whole feature as too confusing.
   (Unless they have somehow divined that they are having RDM trouble
   in which case they may flail at random experimenting with various
   options.)

   Again, the effect therefore is that knowledgeable users might be
   able to do better, but for most users this is just yet another piece
   of docs for some feature they don't want to use.

   While I'm not entirely comfortable with accepting documentation
   which reduces the overall readability and usefulness of the manual,
   I think this is a relatively minor objection which I am prepared to
   overlook.

I agree doc is very important to user. And I have to admit I need to improve this definitely. But if you think I don't write them very well, I think we can have a better way to help me do this. Please just list all key points as question pattern like this,

#1. Definition
#2. Goal
#3. Default setting
#4. How to use
#5. Risk
#6. A good example
...

At least I can try to first cover all necessary information, and then refine each sentence.


   Of course there is some opportunity for improving the documentation
   during the freeze.


* On the question of option naming, `strategy' vs `type':

   `type' was definitely wrong.  It may be that a better name than
   `strategy' would be correct.  This depends on the contemplated
   direction for future expansion.

   Sadly, I do not expect that further discussion is going to
   illuminate this further.  `strategy' will do.


* On the question of option naming, `none' vs `ignore':

   I asked whether the submitter agreed that `none' should be renamed
   `ignore'.  I have not received a clear opinion.  Instead, the
   submitter indicated a willingness to change this on my request.  the
   latest resubmission just did the rename.

I think replied to you on IRC. I just thought they're optional and reasonable, so do you mean this sort of answer is not good to you? But in some cases its not easy to select which one is best.


   The purpose of asking `do you agree', in this way, is to try to help
   the submitters and the maintainers come up with the best answers.

   Note that it is a fundamental assumption of the patch review process
   that the submitter understands the design and implementation
   decisions embodied in the patchset.  The submitter needs to be able
   to respond to suggestions with evaluations, not simply acquiescence.
   (If it happens that some of the decisions were made by someone else,
   the submitter needs to 1. state this clearly where relevant and
   2. either consult the designers/authors, or if they aren't
   available, reverse-engineer the intent.)

   In the absence of a clear statement of the submitter's own opinion,
   I remain doubtful that this rename was correct.  But, I don't think
   it important enough to make any more fuss about.


* On the question of option naming, the `reserve='.

   Ian Campbell points out that the API structure for `[rdm_]reserve'
   as submitted is anomalous.  I agree with him.  The existing
   API and config file arrangements are rather too confusing.

   Please change `reserve' to `policy', in the following places:

   * In the xl rdm config parsing, `reserve=' should be `policy='.
   * In the xl pci config parsing, `rdm_reserve=' should be
     `rdm_policy='.
   * The type `libxl_rdm_reserve_flag' should be `libxl_rdm_policy'.
   * The field name `reserve' in `libxl_rdm_reserve' should be
     `policy'.


Yes, we need to do this better. But this kind of argument really shouldn't finish just one week.

Just please go back to look at all emails started from our design (2014/12/26) to this revision, I believe all tool maintainers are CCed. And especially, these option name were changed a lot of times...

"flag" -> "reserve" -> "policy"
"force" -> "strict"
....

Why didn't you guys say anything so long time? If you guys tried to give us this kind of comments or suggestions as early as possible, I believe you should can get that expected answer to your question.

But now this make both of us frustrated.

Thanks
Tiejun


I think that with these changes I will be able to ack the remaining
tools parts of this series, and drop my objections to the parts acked
by Wei.

I can't speak for the hypervisor side, which I haven't really looked
at.

Ian.



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.