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

["Chen, Tiejun" <tiejun.chen@intel.com>]

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