Re: [Xen-devel] [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Jul 29, 2016 at 10:27 AM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/07/16 15:21, Tamas K Lengyel wrote:
>
> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@xxxxxxx> wrote:
>>
>>
>>
>> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>>
>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@xxxxxxx>
>>> wrote:
>>>>
>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>>> This patch is doing more than it is claimed in the commit message.
>>>>
>>>> In general, moving the code and introducing changes within the same
>>>> patch
>>>> should really be avoided. So please split it in 2 patches.
>>>
>>>
>>> Well, the changes are largely cosmetic so doing a whole separate patch
>>> IMHO is an overkill. How about adjusting the commit message to
>>> something like "sanitize code surrounding sending mem_access
>>> vm_events" to better describe the adjustments made in this patch?
>>
>>
>> I think the wiki page "Submitting Xen Project patches" [1] should answer
>> to your question.
>>
>> If not, trivial patches are easy to review, merging multiple trivial
>> patches in a single patch is not. Moving code and at the same time as
>> changing the behavior is fairly difficult to review because it hides the
>> real modifications.
>>
>> Regards,
>>
>> [1]
>> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>>
>
> The behavior didn't change at all, this whole patch is code sanitization.
> It's not worth doing a separate patch for each minor change. The few change
> on the arm side is the vm_event request allocation going from xzalloc to
> stack based and using monitor_traps now in a split-out function. It really
> should be no problem reviewing it. Even Andrew requested minor adjustments
> to be included in this patch. Anyway, I'm not looking to change this into a
> series. If it's a no go from your side I'm just going to cut down the ARM
> side sanitization to the bare minimum of using monitor_traps as the rest
> just does not worth the effort.
>
>
> To offer an alternative opinion.
>
> Looking at this patch, I personally would have a hard time justifying
> breaking it down any further.  Each of the changes are closely related.
>
> However, the commit message could be a lot clearer about which steps are
> taken.  If I were writing the commit message, I would go with something a
> bit more like this:
>
> ----8<----
> The two functions monitor_traps and mem_access_send_req duplicate some of
> the same functionality. The mem_access_send_req however leaves a lot of the
> standard vm_event fields to be filled by other functions.
>
> Remove mem_access_send_req() completely, making use of monitor_traps() to
> put requests into the monitor ring.  This in turn causes some cleanup around
> the old callsites of mem_access_send_req(), and on ARM, the introduction of
> the __p2m_mem_access_send_req() helper to fill in common mem_access
> information.
>
> Finally, this change identifies that errors from mem_access_send_req() were
> never checked.  As errors constitute a problem with the monitor ring,
> crashing the domain is the most appropriate action to take.
> ----8<----
>
> If in doubt, always spell out each of the changes, and their logical
> relationships.  If nothing else, it helps people trying to review the patch.
>

Thanks and I agree, adjusting the commit message is what I would think
would make sense as well here (and what I offered as an alternative to
breaking down the series into tiny patches).

Tamas

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