Re: [Xen-devel] [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.05.17 at 17:28, <olekstysh@xxxxxxxxx> wrote:
> Hi, Jan.
> 
> On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 16.05.17 at 14:48, <olekstysh@xxxxxxxxx> wrote:
>>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 15.05.17 at 12:43, <olekstysh@xxxxxxxxx> wrote:
>>>>> Indeed, there was some misunderstanding from my side on this.
>>>>> Let me elaborate a bit more on this:
>>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>>>> addressed, so at least I will have them back in the patch
>>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>>>> it makes me lots of work to do this change
>>>>> properly, so this is not only the question of testing the code, but rather
>>>>> having it written.
>>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>>>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>>>> in any way.
>>>>>
>>>>> That being said, can we postpone implementation of the *optimizations*
>>>>> in question
>>>>> and have those as a separate activity?
>>>>> Or if these *optimizations* must be present in the current patch
>>>>> series, could you, please, provide me with some hints how
>>>>> these TODO should be properly implemented?
>>>>
>>>> I'm puzzled. When I first commented on these TODOs I did say
>>>> "While I appreciate this not being done in the already large patch,
>>>> I don't think such a TODO should be left around. If need be (e.g.
>>>> because you can't test the change), get in touch with the
>>>> maintainer(s)." Of course the "e.g." extends to the actual
>>>> implementation. IOW I'm not saying you need to do this work
>>>> immediately and all by yourself, but there should be a clear plan
>>>> on getting these items addressed. We shouldn't ship several
>>>> releases with them still present. I'm sorry this hits you, but we've
>>>> had too bad experience in the past with people leaving todo or
>>>> fixme notes in the code, perhaps even promising to address them
>>>> without ever doing so.
>>> I see. You are right about leaving TODO)
>>> Don't mind to get these items addressed *within current patch series*
>>> as separate patch or patches.
>>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
>>> leave SMMU for myself.
>>>
>>> Could you, please, provide me with some hints how these TODO should be
>>> properly implemented?
>>
>> I have to admit that I don't really understand the request. Quite
>> clearly we want to use large pages in the case that hardware
>> supports them.
>>
>>> I was thinking I can even just squash *pages with *page and send you a
>>> draft as we need to start from somewhere.
>>
>> I'm afraid I've lost too much of the context to see what you mean
>> here.
> Sorry if I was unclear.
> 
> At the moment patch contains three TODOs in the following files:
> 1. a/xen/drivers/passthrough/vtd/iommu.c
> 2. a/xen/drivers/passthrough/amd/iommu_map.c
> 3. a/xen/drivers/passthrough/arm/smmu.c
> 
> And the *optimization* which I mentioned in that TODO is same for all
> three files.
> +/* TODO: Optimize by squashing map_pages/unmap_pages with
> map_page/unmap_page */
> 
> I think that I could try to address this TODO by myself as I imagine
> it should be addressed and send you a draft or post RFC patch.
> As the result of this RFC patch we would have map_pages/unmap_pages
> callbacks only, but still iterate 4K pages.
> 
> We need to start from somewhere and this patch would be a base point
> for continue optimizing.
> What do you think? Or you have another opinion?

Well, yes, this would reduce the scope of the TODO, but no, it
wouldn't eliminate it. After all the primary goal (from my
perspective) of adding the order parameter is to make use of
large pages whenever possible. And as said before - it's not like
it has to be you who does the work, but I'd sort of expect that
whoever introduces TODOs at least tries to arrange for them
being taken care of (unless e.g. they affect exotic situations
only), for example by pulling in the respective maintainers.

Jan

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