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@lists.xen.org
http://lists.xen.org/xen-devel