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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Jan

On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 15.05.17 at 12:43, <olekstysh@xxxxxxxxx> wrote:
>> On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 12.05.17 at 18:25, <olekstysh@xxxxxxxxx> wrote:
>>>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>> On 12.05.17 at 17:50, <olekstysh@xxxxxxxxx> wrote:
>>>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>>>> On 10.05.17 at 16:03, <olekstysh@xxxxxxxxx> wrote:
>>>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, 
>>>>>>>> unsigned long gfn)
>>>>>>>>      return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with 
>>>>>>>> map_page/unmap_page */
>>>>>>>
>>>>>>> Looking over the titles of the rest of this series it doesn't look like
>>>>>>> you're eliminating this TODO later. 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).
>>>>>> I will drop this TODO everywhere.
>>>>>
>>>>> By "drop" you mean "address" or really just "drop"?
>>>> I meant just drop.
>>>
>>> Then I'm sorry, but no, this is not a way to address the comment I've
>>> made.
>>
>> 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?
Or
I was thinking I can even just squash *pages with *page and send you a
draft as we need to start from somewhere.
What do you think?

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

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