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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.


* 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.

  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.

  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'.


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