Re: [virtio-comment] [PATCH v3 2/3] content: Document balloon feature page poison

[David Hildenbrand <david@redhat.com>]

On 26.05.20 16:50, Alexander Duyck wrote:
> On Tue, May 26, 2020 at 1:24 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 20.05.20 18:25, Alexander Duyck wrote:
>>> On Wed, May 20, 2020 at 2:28 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 20.05.20 04:02, Alexander Duyck wrote:
>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>
>>>>> Page poison provides a way for the guest to notify the host of the content
>>>>> expected to be found in pages when they are added back to the guest after
>>>>> being discarded. The feature currently doesn't apply to the existing
>>>>> balloon features, however it will apply to an upcoming feature, free page
>>>>> reporting. Add documentation for the page poison feature describing the
>>>>> basic functionality and requirements.
>>>>>
>>>>
>>>> I would rephrase this, starting what it does *without* free page
>>>> reporting (which is not "provides a way for the guest to notify ..."),
>>>> and then eventually how this feature will also be used in the future as
>>>> well with free page reporting.
>>>
>>> Below is a rewrite on this description. I'm thinking that we can
>>> probably call out the advantage to free page reporting in a different
>>> way. Basically with the page poison feature we know a few things about
>>> the behavior and I have called them out in the new patch description:
>>>
>>> Page poison provides a way for the guest to notify the host that it is
>>> initializing or poisoning freed pages with some specific poison value. As a
>>> result of this we can infer a couple traits about the guest:
>>>
>>> 1. Free pages will contain a specific pattern within the guest.
>>> 2. Modifying free pages from this value may cause an error in the guest.
>>> 3. Pages will be immediately written to by the driver when deflated.
>>>
>>> There are currently no existing features that make use of this data. In the
>>> upcoming feature free page reporting we will need to make use of this to
>>> identify if we can evict pages from the guest without causing data
>>> corruption.
>>>
>>> Add documentation for the page poison feature describing the basic
>>> functionality and requirements.
>>>
> 
> [...]
> 
>>>>> +
>>>>> +If the guest is not initializing or poisoning freed pages it should reject
>>>>
>>>> Sometimes you use "write to pages after deflating", here you use "freed
>>>> pages"
>>>
>>> So when I am referencing "freed pages" I am talking about all free
>>> memory, while when I refer to "pages after deflating" I am talking
>>> about pages coming out of the balloon.
>>>
>>> My thought is that there maybe be additional uses for "poison_val" be
>>> to feed it into some future use other than just the balloon portion of
>>> the deflation. Basically what this is telling us is that we could look
>>> for a pattern of pages containing nothing but poison_val if we wanted
>>> to do some sort of same page merging, or maybe define something to
>>> optimize migration by defining a poison page similar to a zero page
>>> that could be used to reduce migration overhead in the future.
>>>
>>>>> +the VIRTIO_BALLOON_F_PAGE_POISON feature.
>>>>> +
>>>>> +If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the guest
>>>>> +will place the expected poison value into the \field{poison_val}
>>>>
>>>> again, "expected" is misleading in the context of this patch only.
>>>
>>> I will rewrite this statement at follows:
>>>   If VIRTIO_BALLOON_F_PAGE_POISON feature has been negotiated, the driver
>>>   will place the initialization and/or poison value into the \field{poison_val}
>>>   configuration field data.
>>>
>>> I think I might strengthen things a bit as well. In the driver
>>> normative section I think I might add the following:
>>>   The driver MUST initialize and/or poison the deflated pages with
>>>   \field{poison_val} when they are reused by the driver.
>>>
>>
>> Maybe simplify that whole "initialize and/or poison " handling across
>> this patch to "initialize with \field{poison_val}" - if the
>> initialization is used for poisoning or initialization doesn't matter
>> from spec POV.
>>
>> In general looks good to me, I'll have another look at the full result.
> 
> Okay, I will go through and try to flush out the "and/or poison" in
> favor of just calling out the initialization.
> 
>> Still wondering what to do with free page hinting ... in the meantime
>> I'll have a look at free page reporting :)
> 
> The problem is it is already out there so I worry we wouldn't ever be
> able to get rid of it. At most we could deprecate it, but we are still
> stuck with it consuming bit resources and such.

Yeah, that's not an issue, they will simply turn to dead bits with
minimal documentation. I just don't see us fixing/supporting that
feature, really. Let's see what @MST things when he has time to look
into this.

-- 
Thanks,

David / dhildenb