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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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