[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.