Let's check by traversing busy system RAM resources instead, to avoid
relying on memory block states.
Don't use walk_system_ram_range(), as that works on pages and we want to
use the bare addresses we have easily at hand.
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Cc: Pankaj Gupta <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
drivers/virtio/virtio_mem.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index b3eebac7191f..6bbd1cfd10d3 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -1749,6 +1749,20 @@ static void virtio_mem_delete_resource(struct virtio_mem *vm)
vm->parent_resource = NULL;
}
+static int virtio_mem_range_has_system_ram(struct resource *res, void *arg)
+{
+ return 1;
+}
+
+static bool virtio_mem_has_memory_added(struct virtio_mem *vm)
+{
+ const unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+
+ return walk_iomem_res_desc(IORES_DESC_NONE, flags, vm->addr,
+ vm->addr + vm->region_size, NULL,
+ virtio_mem_range_has_system_ram) == 1;
+}
+
static int virtio_mem_probe(struct virtio_device *vdev)
{
struct virtio_mem *vm;
@@ -1870,10 +1884,7 @@ static void virtio_mem_remove(struct virtio_device *vdev)
* the system. And there is no way to stop the driver/device from going
* away. Warn at least.
*/
- if (vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE] ||
- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE_PARTIAL] ||
- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE] ||
- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE_PARTIAL]) {
+ if (virtio_mem_has_memory_added(vm)) {
dev_warn(&vdev->dev, "device still has system memory added\n");
} else {
virtio_mem_delete_resource(vm);
--
2.26.2
On Mon, Oct 12, 2020 at 02:52:59PM +0200, David Hildenbrand wrote:
>Let's check by traversing busy system RAM resources instead, to avoid
>relying on memory block states.
>
>Don't use walk_system_ram_range(), as that works on pages and we want to
>use the bare addresses we have easily at hand.
>
>Cc: "Michael S. Tsirkin" <[email protected]>
>Cc: Jason Wang <[email protected]>
>Cc: Pankaj Gupta <[email protected]>
>Signed-off-by: David Hildenbrand <[email protected]>
>---
> drivers/virtio/virtio_mem.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>index b3eebac7191f..6bbd1cfd10d3 100644
>--- a/drivers/virtio/virtio_mem.c
>+++ b/drivers/virtio/virtio_mem.c
>@@ -1749,6 +1749,20 @@ static void virtio_mem_delete_resource(struct virtio_mem *vm)
> vm->parent_resource = NULL;
> }
>
>+static int virtio_mem_range_has_system_ram(struct resource *res, void *arg)
>+{
>+ return 1;
>+}
>+
>+static bool virtio_mem_has_memory_added(struct virtio_mem *vm)
>+{
>+ const unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>+
>+ return walk_iomem_res_desc(IORES_DESC_NONE, flags, vm->addr,
>+ vm->addr + vm->region_size, NULL,
>+ virtio_mem_range_has_system_ram) == 1;
>+}
>+
> static int virtio_mem_probe(struct virtio_device *vdev)
> {
> struct virtio_mem *vm;
>@@ -1870,10 +1884,7 @@ static void virtio_mem_remove(struct virtio_device *vdev)
> * the system. And there is no way to stop the driver/device from going
> * away. Warn at least.
> */
>- if (vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE_PARTIAL] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE_PARTIAL]) {
>+ if (virtio_mem_has_memory_added(vm)) {
I am not sure this would be more efficient.
> dev_warn(&vdev->dev, "device still has system memory added\n");
> } else {
> virtio_mem_delete_resource(vm);
BTW, I got one question during review.
Per my understanding, there are 4 states of a virtio memory block
* OFFLINE[_PARTIAL]
* ONLINE[_PARTIAL]
While, if my understanding is correct, those two offline states are transient.
If the required range is onlined, the state would be change to
ONLINE[_PARTIAL] respectively. If it is not, the state is reverted to UNUSED
or PLUGGED.
What I am lost is why you do virtio_mem_mb_remove() on OFFLINE_PARTIAL memory
block? Since we wait for the workqueue finish its job.
Also, during virtio_mem_remove(), we just handle OFFLINE_PARTIAL memory block.
How about memory block in other states? It is not necessary to remove
ONLINE[_PARTIAL] memroy blocks?
Thanks in advance, since I may missed some concepts.
>--
>2.26.2
--
Wei Yang
Help you, Help me
On 15.10.20 10:28, Wei Yang wrote:
> On Mon, Oct 12, 2020 at 02:52:59PM +0200, David Hildenbrand wrote:
>> Let's check by traversing busy system RAM resources instead, to avoid
>> relying on memory block states.
>>
>> Don't use walk_system_ram_range(), as that works on pages and we want to
>> use the bare addresses we have easily at hand.
>>
>> Cc: "Michael S. Tsirkin" <[email protected]>
>> Cc: Jason Wang <[email protected]>
>> Cc: Pankaj Gupta <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>> ---
>> drivers/virtio/virtio_mem.c | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>> index b3eebac7191f..6bbd1cfd10d3 100644
>> --- a/drivers/virtio/virtio_mem.c
>> +++ b/drivers/virtio/virtio_mem.c
>> @@ -1749,6 +1749,20 @@ static void virtio_mem_delete_resource(struct virtio_mem *vm)
>> vm->parent_resource = NULL;
>> }
>>
>> +static int virtio_mem_range_has_system_ram(struct resource *res, void *arg)
>> +{
>> + return 1;
>> +}
>> +
>> +static bool virtio_mem_has_memory_added(struct virtio_mem *vm)
>> +{
>> + const unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>> +
>> + return walk_iomem_res_desc(IORES_DESC_NONE, flags, vm->addr,
>> + vm->addr + vm->region_size, NULL,
>> + virtio_mem_range_has_system_ram) == 1;
>> +}
>> +
>> static int virtio_mem_probe(struct virtio_device *vdev)
>> {
>> struct virtio_mem *vm;
>> @@ -1870,10 +1884,7 @@ static void virtio_mem_remove(struct virtio_device *vdev)
>> * the system. And there is no way to stop the driver/device from going
>> * away. Warn at least.
>> */
>> - if (vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE] ||
>> - vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE_PARTIAL] ||
>> - vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE] ||
>> - vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE_PARTIAL]) {
>> + if (virtio_mem_has_memory_added(vm)) {
>
> I am not sure this would be more efficient.
In general, no. However, this is a preparation for Big Block Mode, which
won't have memory block states.
(this path only triggers when unloading the driver - which most probably
only ever happens during my testing ... :) and we don't really care
about performance there)
>
>> dev_warn(&vdev->dev, "device still has system memory added\n");
>> } else {
>> virtio_mem_delete_resource(vm);
>
> BTW, I got one question during review.
>
> Per my understanding, there are 4 states of a virtio memory block
>
> * OFFLINE[_PARTIAL]
> * ONLINE[_PARTIAL]
>
> While, if my understanding is correct, those two offline states are transient.
> If the required range is onlined, the state would be change to
> ONLINE[_PARTIAL] respectively. If it is not, the state is reverted to UNUSED
> or PLUGGED.
Very right.
>
> What I am lost is why you do virtio_mem_mb_remove() on OFFLINE_PARTIAL memory
> block? Since we wait for the workqueue finish its job.
That's an interesting corner case. Assume you have a 128MB memory block
but only 64MB are plugged.
As long as we have our online_pages callback in place, we can hinder the
unplugged 64MB from getting exposed to the buddy
(virtio_mem_online_page_cb()). However, once we unloaded the driver,
this is no longer the case. If someone would online that memory block,
we would expose unplugged memory to the buddy - very bad.
So we have to remove these partially plugged, offline memory blocks when
losing control over them.
I tried to document that via:
"After we unregistered our callbacks, user space can online partially
plugged offline blocks. Make sure to remove them."
>
> Also, during virtio_mem_remove(), we just handle OFFLINE_PARTIAL memory block.
> How about memory block in other states? It is not necessary to remove
> ONLINE[_PARTIAL] memroy blocks?
Blocks that are fully plugged (ONLINE or OFFLINE) can get
onlined/offlined without us having to care. Works fine - we only have to
care about partially plugged blocks.
While we *could* unplug OFFLINE blocks, there is no way we can
deterministically offline+remove ONLINE blocks. So that memory has to
stay, even after we unloaded the driver (similar to the dax/kmem driver).
ONLINE_PARTIAL is already taken care of: it cannot get offlined anymore,
as we still hold references to these struct pages
(virtio_mem_set_fake_offline()), and as we no longer have the memory
notifier in place, we can no longer agree to offline this memory (when
going_offline).
I tried to document that via
"After we unregistered our callbacks, user space can no longer offline
partially plugged online memory blocks. No need to worry about them."
>
> Thanks in advance, since I may missed some concepts.
(force) driver unloading is a complicated corner case.
Thanks!
--
Thanks,
David / dhildenb
On Thu, Oct 15, 2020 at 10:50:27AM +0200, David Hildenbrand wrote:
[...]
>>
>>> dev_warn(&vdev->dev, "device still has system memory added\n");
>>> } else {
>>> virtio_mem_delete_resource(vm);
>>
>> BTW, I got one question during review.
>>
>> Per my understanding, there are 4 states of a virtio memory block
>>
>> * OFFLINE[_PARTIAL]
>> * ONLINE[_PARTIAL]
>>
>> While, if my understanding is correct, those two offline states are transient.
>> If the required range is onlined, the state would be change to
>> ONLINE[_PARTIAL] respectively. If it is not, the state is reverted to UNUSED
>> or PLUGGED.
>
>Very right.
>
>>
>> What I am lost is why you do virtio_mem_mb_remove() on OFFLINE_PARTIAL memory
>> block? Since we wait for the workqueue finish its job.
I have tried to understand the logic, while still have some confusion.
>
>That's an interesting corner case. Assume you have a 128MB memory block
>but only 64MB are plugged.
Since we just plug a part of memory block, this state is OFFLINE_PARTIAL
first. But then we would add these memory and online it. This means the state
of this memory block is ONLINE_PARTIAL.
When this state is changed to OFFLINE_PARTIAL again?
>
>As long as we have our online_pages callback in place, we can hinder the
>unplugged 64MB from getting exposed to the buddy
>(virtio_mem_online_page_cb()). However, once we unloaded the driver,
Yes,
virtio_mem_set_fake_offline() would __SetPageOffline() to those pages.
>this is no longer the case. If someone would online that memory block,
>we would expose unplugged memory to the buddy - very bad.
>
Per my understanding, at this point of time, the memory block is at online
state. Even part of it is set to *fake* offline.
So how could user trigger another online from sysfs interface?
>So we have to remove these partially plugged, offline memory blocks when
>losing control over them.
>
>I tried to document that via:
>
>"After we unregistered our callbacks, user space can online partially
>plugged offline blocks. Make sure to remove them."
>
>>
>> Also, during virtio_mem_remove(), we just handle OFFLINE_PARTIAL memory block.
>> How about memory block in other states? It is not necessary to remove
>> ONLINE[_PARTIAL] memroy blocks?
>
>Blocks that are fully plugged (ONLINE or OFFLINE) can get
>onlined/offlined without us having to care. Works fine - we only have to
>care about partially plugged blocks.
>
>While we *could* unplug OFFLINE blocks, there is no way we can
>deterministically offline+remove ONLINE blocks. So that memory has to
>stay, even after we unloaded the driver (similar to the dax/kmem driver).
For OFFLINE memory blocks, would that leave the situation:
Guest doesn't need those pages, while host still maps them?
>
>ONLINE_PARTIAL is already taken care of: it cannot get offlined anymore,
>as we still hold references to these struct pages
>(virtio_mem_set_fake_offline()), and as we no longer have the memory
>notifier in place, we can no longer agree to offline this memory (when
>going_offline).
>
Ok, I seems to understand the logic now.
But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
How they hold reference to struct page?
>I tried to document that via
>
>"After we unregistered our callbacks, user space can no longer offline
>partially plugged online memory blocks. No need to worry about them."
>
>
>>
>> Thanks in advance, since I may missed some concepts.
>
>(force) driver unloading is a complicated corner case.
>
>Thanks!
>
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
>> That's an interesting corner case. Assume you have a 128MB memory block
>> but only 64MB are plugged.
>
> Since we just plug a part of memory block, this state is OFFLINE_PARTIAL
> first. But then we would add these memory and online it. This means the state
> of this memory block is ONLINE_PARTIAL.
>
> When this state is changed to OFFLINE_PARTIAL again?
Please note that memory onlining is *completely* controllable by user
space. User space can offline/online memory blocks as it wants. Not
saying this might actually be the right thing to do - but we cannot
trust that user space does the right thing.
So at any point in time, you have to assume that
a) added memory might not get onlined
b) previously onlined memory might get offlined
c) previously offline memory might get onlined
>
>>
>> As long as we have our online_pages callback in place, we can hinder the
>> unplugged 64MB from getting exposed to the buddy
>> (virtio_mem_online_page_cb()). However, once we unloaded the driver,
>
> Yes,
>
> virtio_mem_set_fake_offline() would __SetPageOffline() to those pages.
>
>> this is no longer the case. If someone would online that memory block,
>> we would expose unplugged memory to the buddy - very bad.
>>
>
> Per my understanding, at this point of time, the memory block is at online
> state. Even part of it is set to *fake* offline.
>
> So how could user trigger another online from sysfs interface?
Assume we added a partially plugged memory block, which is now offline.
Further assume user space did not online the memory block (e.g., no udev
rules).
User space could happily online the block after unloading the driver.
Again, we have to assume user space could do crazy things.
>
>> So we have to remove these partially plugged, offline memory blocks when
>> losing control over them.
>>
>> I tried to document that via:
>>
>> "After we unregistered our callbacks, user space can online partially
>> plugged offline blocks. Make sure to remove them."
>>
>>>
>>> Also, during virtio_mem_remove(), we just handle OFFLINE_PARTIAL memory block.
>>> How about memory block in other states? It is not necessary to remove
>>> ONLINE[_PARTIAL] memroy blocks?
>>
>> Blocks that are fully plugged (ONLINE or OFFLINE) can get
>> onlined/offlined without us having to care. Works fine - we only have to
>> care about partially plugged blocks.
>>
>> While we *could* unplug OFFLINE blocks, there is no way we can
>> deterministically offline+remove ONLINE blocks. So that memory has to
>> stay, even after we unloaded the driver (similar to the dax/kmem driver).
>
> For OFFLINE memory blocks, would that leave the situation:
>
> Guest doesn't need those pages, while host still maps them?
Yes, but the guest could online the memory and make use of it.
(again, whoever decides to unload the driver better be knowing what he does)
To do it even more cleanly, we would
a) Have to remove completely plugged offline blocks (not done)
b) Have to remove partially plugged offline blocks (done)
c) Actually send unplug requests to the hypervisor
Right now, only b) is done, because it might actually cause harm (as
discussed). However, the problem is, that c) might actually fail.
Long short: we could add a) if it turns out to be a real issue. But
than, unloading the driver isn't really suggested, the current
implementation just "keeps it working without crashes" - and I guess
that's good enough for now.
>
>>
>> ONLINE_PARTIAL is already taken care of: it cannot get offlined anymore,
>> as we still hold references to these struct pages
>> (virtio_mem_set_fake_offline()), and as we no longer have the memory
>> notifier in place, we can no longer agree to offline this memory (when
>> going_offline).
>>
>
> Ok, I seems to understand the logic now.
>
> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
> How they hold reference to struct page?
Sorry, I should have given you the right pointer. (similar to my other
reply)
We hold a reference either via
1. alloc_contig_range()
2. memmap init code, when not calling generic_online_page().
So these fake-offline pages can never be actually offlined, because we
no longer have the memory notifier registered to fix that up.
--
Thanks,
David / dhildenb
On Fri, Oct 16, 2020 at 11:11:24AM +0200, David Hildenbrand wrote:
>>> That's an interesting corner case. Assume you have a 128MB memory block
>>> but only 64MB are plugged.
>>
>> Since we just plug a part of memory block, this state is OFFLINE_PARTIAL
>> first. But then we would add these memory and online it. This means the state
>> of this memory block is ONLINE_PARTIAL.
>>
>> When this state is changed to OFFLINE_PARTIAL again?
>
>Please note that memory onlining is *completely* controllable by user
>space. User space can offline/online memory blocks as it wants. Not
>saying this might actually be the right thing to do - but we cannot
>trust that user space does the right thing.
>
>So at any point in time, you have to assume that
>
>a) added memory might not get onlined
>b) previously onlined memory might get offlined
>c) previously offline memory might get onlined
>
>>
>>>
>>> As long as we have our online_pages callback in place, we can hinder the
>>> unplugged 64MB from getting exposed to the buddy
>>> (virtio_mem_online_page_cb()). However, once we unloaded the driver,
>>
>> Yes,
>>
>> virtio_mem_set_fake_offline() would __SetPageOffline() to those pages.
>>
>>> this is no longer the case. If someone would online that memory block,
>>> we would expose unplugged memory to the buddy - very bad.
>>>
>>
>> Per my understanding, at this point of time, the memory block is at online
>> state. Even part of it is set to *fake* offline.
>>
>> So how could user trigger another online from sysfs interface?
>
>Assume we added a partially plugged memory block, which is now offline.
>Further assume user space did not online the memory block (e.g., no udev
>rules).
>
>User space could happily online the block after unloading the driver.
>Again, we have to assume user space could do crazy things.
>
You are right, online memory is not a forced behavior.
>>
>>> So we have to remove these partially plugged, offline memory blocks when
>>> losing control over them.
>>>
>>> I tried to document that via:
>>>
>>> "After we unregistered our callbacks, user space can online partially
>>> plugged offline blocks. Make sure to remove them."
>>>
>>>>
>>>> Also, during virtio_mem_remove(), we just handle OFFLINE_PARTIAL memory block.
>>>> How about memory block in other states? It is not necessary to remove
>>>> ONLINE[_PARTIAL] memroy blocks?
>>>
>>> Blocks that are fully plugged (ONLINE or OFFLINE) can get
>>> onlined/offlined without us having to care. Works fine - we only have to
>>> care about partially plugged blocks.
>>>
>>> While we *could* unplug OFFLINE blocks, there is no way we can
>>> deterministically offline+remove ONLINE blocks. So that memory has to
>>> stay, even after we unloaded the driver (similar to the dax/kmem driver).
>>
>> For OFFLINE memory blocks, would that leave the situation:
>>
>> Guest doesn't need those pages, while host still maps them?
>
>Yes, but the guest could online the memory and make use of it.
>
>(again, whoever decides to unload the driver better be knowing what he does)
>
>To do it even more cleanly, we would
>
>a) Have to remove completely plugged offline blocks (not done)
>b) Have to remove partially plugged offline blocks (done)
>c) Actually send unplug requests to the hypervisor
>
>Right now, only b) is done, because it might actually cause harm (as
>discussed). However, the problem is, that c) might actually fail.
>
>Long short: we could add a) if it turns out to be a real issue. But
>than, unloading the driver isn't really suggested, the current
>implementation just "keeps it working without crashes" - and I guess
>that's good enough for now.
>
>>
>>>
>>> ONLINE_PARTIAL is already taken care of: it cannot get offlined anymore,
>>> as we still hold references to these struct pages
>>> (virtio_mem_set_fake_offline()), and as we no longer have the memory
>>> notifier in place, we can no longer agree to offline this memory (when
>>> going_offline).
>>>
>>
>> Ok, I seems to understand the logic now.
>>
>> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
>> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
>> How they hold reference to struct page?
>
>Sorry, I should have given you the right pointer. (similar to my other
>reply)
>
>We hold a reference either via
>
>1. alloc_contig_range()
I am not familiar with this one, need to spend some time to look into.
>2. memmap init code, when not calling generic_online_page().
I may miss some code here. Before online pages, memmaps are allocated in
section_activate(). They are supposed to be zero-ed. (I don't get the exact
code line.) I am not sure when we grab a refcount here.
>
>So these fake-offline pages can never be actually offlined, because we
>no longer have the memory notifier registered to fix that up.
>
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
>>> Ok, I seems to understand the logic now.
>>>
>>> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
>>> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
>>> How they hold reference to struct page?
>>
>> Sorry, I should have given you the right pointer. (similar to my other
>> reply)
>>
>> We hold a reference either via
>>
>> 1. alloc_contig_range()
>
> I am not familiar with this one, need to spend some time to look into.
Each individual page will have a pagecount of 1.
>
>> 2. memmap init code, when not calling generic_online_page().
>
> I may miss some code here. Before online pages, memmaps are allocated in
> section_activate(). They are supposed to be zero-ed. (I don't get the exact
> code line.) I am not sure when we grab a refcount here.
Best to refer to __init_single_page() -> init_page_count().
Each page that wasn't onlined via generic_online_page() has a refcount
of 1 and looks like allocated.
--
Thanks,
David / dhildenb
On Mon, Oct 12, 2020 at 02:52:59PM +0200, David Hildenbrand wrote:
>Let's check by traversing busy system RAM resources instead, to avoid
>relying on memory block states.
>
>Don't use walk_system_ram_range(), as that works on pages and we want to
>use the bare addresses we have easily at hand.
>
>Cc: "Michael S. Tsirkin" <[email protected]>
>Cc: Jason Wang <[email protected]>
>Cc: Pankaj Gupta <[email protected]>
>Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Wei Yang <[email protected]>
>---
> drivers/virtio/virtio_mem.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>index b3eebac7191f..6bbd1cfd10d3 100644
>--- a/drivers/virtio/virtio_mem.c
>+++ b/drivers/virtio/virtio_mem.c
>@@ -1749,6 +1749,20 @@ static void virtio_mem_delete_resource(struct virtio_mem *vm)
> vm->parent_resource = NULL;
> }
>
>+static int virtio_mem_range_has_system_ram(struct resource *res, void *arg)
>+{
>+ return 1;
>+}
>+
>+static bool virtio_mem_has_memory_added(struct virtio_mem *vm)
>+{
>+ const unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>+
>+ return walk_iomem_res_desc(IORES_DESC_NONE, flags, vm->addr,
>+ vm->addr + vm->region_size, NULL,
>+ virtio_mem_range_has_system_ram) == 1;
>+}
>+
> static int virtio_mem_probe(struct virtio_device *vdev)
> {
> struct virtio_mem *vm;
>@@ -1870,10 +1884,7 @@ static void virtio_mem_remove(struct virtio_device *vdev)
> * the system. And there is no way to stop the driver/device from going
> * away. Warn at least.
> */
>- if (vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_OFFLINE_PARTIAL] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE] ||
>- vm->nb_mb_state[VIRTIO_MEM_MB_STATE_ONLINE_PARTIAL]) {
>+ if (virtio_mem_has_memory_added(vm)) {
> dev_warn(&vdev->dev, "device still has system memory added\n");
> } else {
> virtio_mem_delete_resource(vm);
>--
>2.26.2
--
Wei Yang
Help you, Help me
On Fri, Oct 16, 2020 at 12:32:50PM +0200, David Hildenbrand wrote:
>>>> Ok, I seems to understand the logic now.
>>>>
>>>> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
>>>> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
>>>> How they hold reference to struct page?
>>>
>>> Sorry, I should have given you the right pointer. (similar to my other
>>> reply)
>>>
>>> We hold a reference either via
>>>
>>> 1. alloc_contig_range()
>>
>> I am not familiar with this one, need to spend some time to look into.
>
>Each individual page will have a pagecount of 1.
>
>>
>>> 2. memmap init code, when not calling generic_online_page().
>>
>> I may miss some code here. Before online pages, memmaps are allocated in
>> section_activate(). They are supposed to be zero-ed. (I don't get the exact
>> code line.) I am not sure when we grab a refcount here.
>
>Best to refer to __init_single_page() -> init_page_count().
>
>Each page that wasn't onlined via generic_online_page() has a refcount
>of 1 and looks like allocated.
>
Thanks, I see the logic.
online_pages()
move_pfn_range_to_zone() --- 1)
online_pages_range() --- 2)
At 1), __init_single_page() would set page count to 1. At 2),
generic_online_page() would clear page count, while the call back would not.
Then I am trying to search the place where un-zero page count prevent offline.
scan_movable_pages() would fail, since this is a PageOffline() and has 1 page
count.
So the GUARD we prevent offline partial-onlined pages is
(PageOffline && page_count)
And your commit aa218795cb5fd583c94f
mm: Allow to offline unmovable PageOffline() pages via MEM_GOING_OFFLINE
is introduced to handle this case.
That's pretty clear now.
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
> Am 17.10.2020 um 00:38 schrieb Wei Yang <[email protected]>:
>
> On Fri, Oct 16, 2020 at 12:32:50PM +0200, David Hildenbrand wrote:
>>>>> Ok, I seems to understand the logic now.
>>>>>
>>>>> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
>>>>> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
>>>>> How they hold reference to struct page?
>>>>
>>>> Sorry, I should have given you the right pointer. (similar to my other
>>>> reply)
>>>>
>>>> We hold a reference either via
>>>>
>>>> 1. alloc_contig_range()
>>>
>>> I am not familiar with this one, need to spend some time to look into.
>>
>> Each individual page will have a pagecount of 1.
>>
>>>
>>>> 2. memmap init code, when not calling generic_online_page().
>>>
>>> I may miss some code here. Before online pages, memmaps are allocated in
>>> section_activate(). They are supposed to be zero-ed. (I don't get the exact
>>> code line.) I am not sure when we grab a refcount here.
>>
>> Best to refer to __init_single_page() -> init_page_count().
>>
>> Each page that wasn't onlined via generic_online_page() has a refcount
>> of 1 and looks like allocated.
>>
>
> Thanks, I see the logic.
>
> online_pages()
> move_pfn_range_to_zone() --- 1)
> online_pages_range() --- 2)
>
> At 1), __init_single_page() would set page count to 1. At 2),
> generic_online_page() would clear page count, while the call back would not.
>
> Then I am trying to search the place where un-zero page count prevent offline.
> scan_movable_pages() would fail, since this is a PageOffline() and has 1 page
> count.
>
> So the GUARD we prevent offline partial-onlined pages is
>
> (PageOffline && page_count)
>
> And your commit aa218795cb5fd583c94f
>
> mm: Allow to offline unmovable PageOffline() pages via MEM_GOING_OFFLINE
>
> is introduced to handle this case.
>
> That's pretty clear now.
>
I‘m happy to see that I am no longer the only person that understands all this magic :)
Thanks for having a look / reviewing!
>> --
>> Thanks,
>>
>> David / dhildenb
>
> --
> Wei Yang
> Help you, Help me
>
On Sat, Oct 17, 2020 at 09:39:38AM +0200, David Hildenbrand wrote:
>
>> Am 17.10.2020 um 00:38 schrieb Wei Yang <[email protected]>:
>>
>> On Fri, Oct 16, 2020 at 12:32:50PM +0200, David Hildenbrand wrote:
>>>>>> Ok, I seems to understand the logic now.
>>>>>>
>>>>>> But how we prevent ONLINE_PARTIAL memory block get offlined? There are three
>>>>>> calls in virtio_mem_set_fake_offline(), while all of them adjust page's flag.
>>>>>> How they hold reference to struct page?
>>>>>
>>>>> Sorry, I should have given you the right pointer. (similar to my other
>>>>> reply)
>>>>>
>>>>> We hold a reference either via
>>>>>
>>>>> 1. alloc_contig_range()
>>>>
>>>> I am not familiar with this one, need to spend some time to look into.
>>>
>>> Each individual page will have a pagecount of 1.
>>>
>>>>
>>>>> 2. memmap init code, when not calling generic_online_page().
>>>>
>>>> I may miss some code here. Before online pages, memmaps are allocated in
>>>> section_activate(). They are supposed to be zero-ed. (I don't get the exact
>>>> code line.) I am not sure when we grab a refcount here.
>>>
>>> Best to refer to __init_single_page() -> init_page_count().
>>>
>>> Each page that wasn't onlined via generic_online_page() has a refcount
>>> of 1 and looks like allocated.
>>>
>>
>> Thanks, I see the logic.
>>
>> online_pages()
>> move_pfn_range_to_zone() --- 1)
>> online_pages_range() --- 2)
>>
>> At 1), __init_single_page() would set page count to 1. At 2),
>> generic_online_page() would clear page count, while the call back would not.
>>
>> Then I am trying to search the place where un-zero page count prevent offline.
>> scan_movable_pages() would fail, since this is a PageOffline() and has 1 page
>> count.
>>
>> So the GUARD we prevent offline partial-onlined pages is
>>
>> (PageOffline && page_count)
>>
>> And your commit aa218795cb5fd583c94f
>>
>> mm: Allow to offline unmovable PageOffline() pages via MEM_GOING_OFFLINE
>>
>> is introduced to handle this case.
>>
>> That's pretty clear now.
>>
>
>I‘m happy to see that I am no longer the only person that understands all this magic :)
Thanks for sharing the magic :-)
>
>Thanks for having a look / reviewing!
>
>>> --
>>> Thanks,
>>>
>>> David / dhildenb
>>
>> --
>> Wei Yang
>> Help you, Help me
>>
--
Wei Yang
Help you, Help me