Re: [PATCH v4] gnttab: defer allocation of status frame tracking array

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2021 12:49, Andrew Cooper wrote:
> On 04/05/2021 09:42, Jan Beulich wrote:
>> This array can be large when many grant frames are permitted; avoid
>> allocating it when it's not going to be used anyway, by doing this only
>> in gnttab_populate_status_frames(). While the delaying of the respective
>> memory allocation adds possible reasons for failure of the respective
>> enclosing operations, there are other memory allocations there already,
>> so callers can't expect these operations to always succeed anyway.
>>
>> As to the re-ordering at the end of gnttab_unpopulate_status_frames(),
>> this is merely to represent intended order of actions (shrink array
>> bound, then free higher array entries). If there were racing accesses,
>> suitable barriers would need adding in addition.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Nack.
> 
> The argument you make says that the grant status frames is "large
> enough" to care about not allocating.  (I frankly disagree, but that
> isn't relevant to my to my nack).
> 
> The change in logic here moves a failure in DOMCTL_createdomain, to
> GNTTABOP_setversion.  We know, from the last minute screwups in XSA-226,
> that there are versions of Windows and Linux in the field, used by
> customers, which will BUG()/BSOD when GNTTABOP_setversion doesn't succeed.
> 
> You're literally adding even more complexity to the grant table, to also
> increase the chance of regressing VMs in the wild.  This is not ok.

To me, arguing like this is not okay. The code should have been written
like this in the first place. There's absolutely no reason to allocate
memory up front which is never going to be used.

> The only form of this patch which is in any way acceptable, is to avoid
> the allocation when you know *in DOMCTL_createdomain* that it will never
> be needed by the VM.  So far, that is from Kconfig and/or the command
> line options.

Well, may I remind you that this is how this patch had started? And
may I further remind you that it was Julien who asked me to convert to
this model? And may I then remind you that I already did suggest that
the two of you should come to an agreement? I'm not going to act as a
moderator in this process. But I insist that it is really bad practice
to drop the ball and leave patches (and alike) hanging in the air.

Just to be clear - in case I don't observe the two of you agreeing on
whichever outcome in the foreseeable future, I'm going to make attempts
to get another opinion from yet another REST maintainer. Since I can
live with both forms of the change (but would prefer the more
aggressive savings as done in this version), I can also live with
whatever 4th opinion would surface. But in case the result was not in
your favor, I'd then consider your Nack overruled.

Jan