As guest_memfd is now managed by RamBlockAttribute with
RamDiscardManager, only block uncoordinated discard.
Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
---
Changes in v5:
- Revert to use RamDiscardManager.
Changes in v4:
- Modify commit message (RamDiscardManager->PrivateSharedManager).
Changes in v3:
- No change.
Changes in v2:
- Change the ram_block_discard_require(false) to
ram_block_coordinated_discard_require(false).
---
system/physmem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index f05f7ff09a..58b7614660 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
}
assert(new_block->guest_memfd < 0);
- ret = ram_block_discard_require(true);
+ ret = ram_block_coordinated_discard_require(true);
if (ret < 0) {
error_setg_errno(errp, -ret,
"cannot set up private guest memory: discard currently blocked");
@@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
* ever develops a need to check for errors.
*/
close(new_block->guest_memfd);
- ram_block_discard_require(false);
+ ram_block_coordinated_discard_require(false);
qemu_mutex_unlock_ramlist();
goto out_free;
}
@@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
if (block->guest_memfd >= 0) {
ram_block_attribute_destroy(block->ram_shared);
close(block->guest_memfd);
- ram_block_discard_require(false);
+ ram_block_coordinated_discard_require(false);
}
g_free(block);
--
2.43.5On 20.05.25 12:28, Chenyi Qiang wrote:
> As guest_memfd is now managed by RamBlockAttribute with
> RamDiscardManager, only block uncoordinated discard.
>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> Changes in v5:
> - Revert to use RamDiscardManager.
>
> Changes in v4:
> - Modify commit message (RamDiscardManager->PrivateSharedManager).
>
> Changes in v3:
> - No change.
>
> Changes in v2:
> - Change the ram_block_discard_require(false) to
> ram_block_coordinated_discard_require(false).
> ---
> system/physmem.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index f05f7ff09a..58b7614660 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> }
> assert(new_block->guest_memfd < 0);
>
> - ret = ram_block_discard_require(true);
> + ret = ram_block_coordinated_discard_require(true);
> if (ret < 0) {
> error_setg_errno(errp, -ret,
> "cannot set up private guest memory: discard currently blocked");
> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> * ever develops a need to check for errors.
> */
> close(new_block->guest_memfd);
> - ram_block_discard_require(false);
> + ram_block_coordinated_discard_require(false);
> qemu_mutex_unlock_ramlist();
> goto out_free;
> }
> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
> if (block->guest_memfd >= 0) {
> ram_block_attribute_destroy(block->ram_shared);
> close(block->guest_memfd);
> - ram_block_discard_require(false);
> + ram_block_coordinated_discard_require(false);
> }
>
> g_free(block);
I think this patch should be squashed into the previous one, then the
story in that single patch is consistent.
--
Cheers,
David / dhildenb
On 5/26/2025 5:08 PM, David Hildenbrand wrote:
> On 20.05.25 12:28, Chenyi Qiang wrote:
>> As guest_memfd is now managed by RamBlockAttribute with
>> RamDiscardManager, only block uncoordinated discard.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v5:
>> - Revert to use RamDiscardManager.
>>
>> Changes in v4:
>> - Modify commit message (RamDiscardManager->PrivateSharedManager).
>>
>> Changes in v3:
>> - No change.
>>
>> Changes in v2:
>> - Change the ram_block_discard_require(false) to
>> ram_block_coordinated_discard_require(false).
>> ---
>> system/physmem.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/system/physmem.c b/system/physmem.c
>> index f05f7ff09a..58b7614660 100644
>> --- a/system/physmem.c
>> +++ b/system/physmem.c
>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>> Error **errp)
>> }
>> assert(new_block->guest_memfd < 0);
>> - ret = ram_block_discard_require(true);
>> + ret = ram_block_coordinated_discard_require(true);
>> if (ret < 0) {
>> error_setg_errno(errp, -ret,
>> "cannot set up private guest memory:
>> discard currently blocked");
>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>> Error **errp)
>> * ever develops a need to check for errors.
>> */
>> close(new_block->guest_memfd);
>> - ram_block_discard_require(false);
>> + ram_block_coordinated_discard_require(false);
>> qemu_mutex_unlock_ramlist();
>> goto out_free;
>> }
>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>> if (block->guest_memfd >= 0) {
>> ram_block_attribute_destroy(block->ram_shared);
>> close(block->guest_memfd);
>> - ram_block_discard_require(false);
>> + ram_block_coordinated_discard_require(false);
>> }
>> g_free(block);
>
>
> I think this patch should be squashed into the previous one, then the
> story in that single patch is consistent.
I think this patch is a gate to allow device assignment with guest_memfd
and want to make it separately. Can we instead add some commit message
in previous one? like:
"Using guest_memfd with vfio is still blocked via
ram_block_discard_disable()/ram_block_discard_require()."
>
On 27.05.25 07:47, Chenyi Qiang wrote:
>
>
> On 5/26/2025 5:08 PM, David Hildenbrand wrote:
>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>> As guest_memfd is now managed by RamBlockAttribute with
>>> RamDiscardManager, only block uncoordinated discard.
>>>
>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>> ---
>>> Changes in v5:
>>> - Revert to use RamDiscardManager.
>>>
>>> Changes in v4:
>>> - Modify commit message (RamDiscardManager->PrivateSharedManager).
>>>
>>> Changes in v3:
>>> - No change.
>>>
>>> Changes in v2:
>>> - Change the ram_block_discard_require(false) to
>>> ram_block_coordinated_discard_require(false).
>>> ---
>>> system/physmem.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/system/physmem.c b/system/physmem.c
>>> index f05f7ff09a..58b7614660 100644
>>> --- a/system/physmem.c
>>> +++ b/system/physmem.c
>>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>>> Error **errp)
>>> }
>>> assert(new_block->guest_memfd < 0);
>>> - ret = ram_block_discard_require(true);
>>> + ret = ram_block_coordinated_discard_require(true);
>>> if (ret < 0) {
>>> error_setg_errno(errp, -ret,
>>> "cannot set up private guest memory:
>>> discard currently blocked");
>>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>>> Error **errp)
>>> * ever develops a need to check for errors.
>>> */
>>> close(new_block->guest_memfd);
>>> - ram_block_discard_require(false);
>>> + ram_block_coordinated_discard_require(false);
>>> qemu_mutex_unlock_ramlist();
>>> goto out_free;
>>> }
>>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>>> if (block->guest_memfd >= 0) {
>>> ram_block_attribute_destroy(block->ram_shared);
>>> close(block->guest_memfd);
>>> - ram_block_discard_require(false);
>>> + ram_block_coordinated_discard_require(false);
>>> }
>>> g_free(block);
>>
>>
>> I think this patch should be squashed into the previous one, then the
>> story in that single patch is consistent.
>
> I think this patch is a gate to allow device assignment with guest_memfd
> and want to make it separately. Can we instead add some commit message
> in previous one? like:
>
> "Using guest_memfd with vfio is still blocked via
> ram_block_discard_disable()/ram_block_discard_require()."
For the title it should probably be something like:
"physmem: support coordinated discarding of RAM with guest_memdfd"
Then explain how we install the RAMDiscardManager that will notify
listeners (esp. vfio).
--
Cheers,
David / dhildenb
On 5/27/2025 7:20 PM, David Hildenbrand wrote:
> On 27.05.25 07:47, Chenyi Qiang wrote:
>>
>>
>> On 5/26/2025 5:08 PM, David Hildenbrand wrote:
>>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>>> As guest_memfd is now managed by RamBlockAttribute with
>>>> RamDiscardManager, only block uncoordinated discard.
>>>>
>>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>>> ---
>>>> Changes in v5:
>>>> - Revert to use RamDiscardManager.
>>>>
>>>> Changes in v4:
>>>> - Modify commit message (RamDiscardManager-
>>>> >PrivateSharedManager).
>>>>
>>>> Changes in v3:
>>>> - No change.
>>>>
>>>> Changes in v2:
>>>> - Change the ram_block_discard_require(false) to
>>>> ram_block_coordinated_discard_require(false).
>>>> ---
>>>> system/physmem.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/system/physmem.c b/system/physmem.c
>>>> index f05f7ff09a..58b7614660 100644
>>>> --- a/system/physmem.c
>>>> +++ b/system/physmem.c
>>>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>> }
>>>> assert(new_block->guest_memfd < 0);
>>>> - ret = ram_block_discard_require(true);
>>>> + ret = ram_block_coordinated_discard_require(true);
>>>> if (ret < 0) {
>>>> error_setg_errno(errp, -ret,
>>>> "cannot set up private guest memory:
>>>> discard currently blocked");
>>>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>> * ever develops a need to check for errors.
>>>> */
>>>> close(new_block->guest_memfd);
>>>> - ram_block_discard_require(false);
>>>> + ram_block_coordinated_discard_require(false);
>>>> qemu_mutex_unlock_ramlist();
>>>> goto out_free;
>>>> }
>>>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>>>> if (block->guest_memfd >= 0) {
>>>> ram_block_attribute_destroy(block->ram_shared);
>>>> close(block->guest_memfd);
>>>> - ram_block_discard_require(false);
>>>> + ram_block_coordinated_discard_require(false);
>>>> }
>>>> g_free(block);
>>>
>>>
>>> I think this patch should be squashed into the previous one, then the
>>> story in that single patch is consistent.
>>
>> I think this patch is a gate to allow device assignment with guest_memfd
>> and want to make it separately. Can we instead add some commit message
>> in previous one? like:
>>
>> "Using guest_memfd with vfio is still blocked via
>> ram_block_discard_disable()/ram_block_discard_require()."
>
> For the title it should probably be something like:
>
> "physmem: support coordinated discarding of RAM with guest_memdfd"
>
> Then explain how we install the RAMDiscardManager that will notify
> listeners (esp. vfio).
Make sense. Will do the squash and change the title. Thanks!
>
On 27/5/25 15:47, Chenyi Qiang wrote:
>
>
> On 5/26/2025 5:08 PM, David Hildenbrand wrote:
>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>> As guest_memfd is now managed by RamBlockAttribute with
>>> RamDiscardManager, only block uncoordinated discard.
>>>
>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>> ---
>>> Changes in v5:
>>> - Revert to use RamDiscardManager.
>>>
>>> Changes in v4:
>>> - Modify commit message (RamDiscardManager->PrivateSharedManager).
>>>
>>> Changes in v3:
>>> - No change.
>>>
>>> Changes in v2:
>>> - Change the ram_block_discard_require(false) to
>>> ram_block_coordinated_discard_require(false).
>>> ---
>>> system/physmem.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/system/physmem.c b/system/physmem.c
>>> index f05f7ff09a..58b7614660 100644
>>> --- a/system/physmem.c
>>> +++ b/system/physmem.c
>>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>>> Error **errp)
>>> }
>>> assert(new_block->guest_memfd < 0);
>>> - ret = ram_block_discard_require(true);
>>> + ret = ram_block_coordinated_discard_require(true);
>>> if (ret < 0) {
>>> error_setg_errno(errp, -ret,
>>> "cannot set up private guest memory:
>>> discard currently blocked");
>>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>>> Error **errp)
>>> * ever develops a need to check for errors.
>>> */
>>> close(new_block->guest_memfd);
>>> - ram_block_discard_require(false);
>>> + ram_block_coordinated_discard_require(false);
>>> qemu_mutex_unlock_ramlist();
>>> goto out_free;
>>> }
>>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>>> if (block->guest_memfd >= 0) {
>>> ram_block_attribute_destroy(block->ram_shared);
>>> close(block->guest_memfd);
>>> - ram_block_discard_require(false);
>>> + ram_block_coordinated_discard_require(false);
>>> }
>>> g_free(block);
>>
>>
>> I think this patch should be squashed into the previous one, then the
>> story in that single patch is consistent.
>
> I think this patch is a gate to allow device assignment with guest_memfd
> and want to make it separately.
It is not good for bisecability - whatever problem 06/10 may have - git bisect will point to this one.
And it is confusing when within the same patchset lines are added and then removed.
And 06/10 (especially after removing LiveMigration checks) and 07/10 are too small and too related to separate. Thanks,
> Can we instead add some commit message
> in previous one? like:
>
> "Using guest_memfd with vfio is still blocked via
> ram_block_discard_disable()/ram_block_discard_require()."
>
>>
>
--
Alexey
On 5/27/2025 3:42 PM, Alexey Kardashevskiy wrote:
>
>
> On 27/5/25 15:47, Chenyi Qiang wrote:
>>
>>
>> On 5/26/2025 5:08 PM, David Hildenbrand wrote:
>>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>>> As guest_memfd is now managed by RamBlockAttribute with
>>>> RamDiscardManager, only block uncoordinated discard.
>>>>
>>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>>> ---
>>>> Changes in v5:
>>>> - Revert to use RamDiscardManager.
>>>>
>>>> Changes in v4:
>>>> - Modify commit message (RamDiscardManager-
>>>> >PrivateSharedManager).
>>>>
>>>> Changes in v3:
>>>> - No change.
>>>>
>>>> Changes in v2:
>>>> - Change the ram_block_discard_require(false) to
>>>> ram_block_coordinated_discard_require(false).
>>>> ---
>>>> system/physmem.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/system/physmem.c b/system/physmem.c
>>>> index f05f7ff09a..58b7614660 100644
>>>> --- a/system/physmem.c
>>>> +++ b/system/physmem.c
>>>> @@ -1916,7 +1916,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>> }
>>>> assert(new_block->guest_memfd < 0);
>>>> - ret = ram_block_discard_require(true);
>>>> + ret = ram_block_coordinated_discard_require(true);
>>>> if (ret < 0) {
>>>> error_setg_errno(errp, -ret,
>>>> "cannot set up private guest memory:
>>>> discard currently blocked");
>>>> @@ -1939,7 +1939,7 @@ static void ram_block_add(RAMBlock *new_block,
>>>> Error **errp)
>>>> * ever develops a need to check for errors.
>>>> */
>>>> close(new_block->guest_memfd);
>>>> - ram_block_discard_require(false);
>>>> + ram_block_coordinated_discard_require(false);
>>>> qemu_mutex_unlock_ramlist();
>>>> goto out_free;
>>>> }
>>>> @@ -2302,7 +2302,7 @@ static void reclaim_ramblock(RAMBlock *block)
>>>> if (block->guest_memfd >= 0) {
>>>> ram_block_attribute_destroy(block->ram_shared);
>>>> close(block->guest_memfd);
>>>> - ram_block_discard_require(false);
>>>> + ram_block_coordinated_discard_require(false);
>>>> }
>>>> g_free(block);
>>>
>>>
>>> I think this patch should be squashed into the previous one, then the
>>> story in that single patch is consistent.
>>
>> I think this patch is a gate to allow device assignment with guest_memfd
>> and want to make it separately.
>
> It is not good for bisecability - whatever problem 06/10 may have - git
> bisect will point to this one.
Bisecability seems not a strong reason, since what problem of patch
04,05,06 may have, git bisect will point to this one as they won't take
effect until allowing coordinated discard
> And it is confusing when within the same patchset lines are added and
> then removed.
> And 06/10 (especially after removing LiveMigration checks) and 07/10 are
> too small and too related to separate. Thanks,
Fair enough. I'll squash it. Thanks for elaboration.
>
>> Can we instead add some commit message
>> in previous one? like:
>>
>> "Using guest_memfd with vfio is still blocked via
>> ram_block_discard_disable()/ram_block_discard_require()."
>>
>>>
>>
>
© 2016 - 2025 Red Hat, Inc.