Re: [PATCH v1 21/29] virtio-mem: memory notifier callbacks are specific to Sub Block Mode (SBM)

2020-10-19 Thread David Hildenbrand 
On 19.10.20 03:57, Wei Yang wrote:
> On Mon, Oct 12, 2020 at 02:53:15PM +0200, David Hildenbrand wrote:
>> Let's rename accordingly.
>>
>> Cc: "Michael S. Tsirkin" 
>> Cc: Jason Wang 
>> Cc: Pankaj Gupta 
>> Signed-off-by: David Hildenbrand 
>> ---
>> drivers/virtio/virtio_mem.c | 29 +++--
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>> index 3a772714fec9..d06c8760b337 100644
>> --- a/drivers/virtio/virtio_mem.c
>> +++ b/drivers/virtio/virtio_mem.c
>> @@ -589,8 +589,8 @@ static bool virtio_mem_contains_range(struct virtio_mem 
>> *vm, uint64_t start,
>>  return start >= vm->addr && start + size <= vm->addr + vm->region_size;
>> }
>>
>> -static int virtio_mem_notify_going_online(struct virtio_mem *vm,
>> -  unsigned long mb_id)
>> +static int virtio_mem_sbm_notify_going_online(struct virtio_mem *vm,
>> +  unsigned long mb_id)
> 
> Look into this patch with "virtio-mem: Big Block Mode (BBM) memory hotplug"
> together, I thought the code is a little "complex".
> 
> The final logic of virtio_mem_memory_notifier_cb() looks like this:
> 
> virtio_mem_memory_notifier_cb()
> if (vm->in_sbm)
>   notify_xxx()
> if (vm->in_sbm)
>   notify_xxx()
> 
> Can we adjust this like
> 
> virtio_mem_memory_notifier_cb()
>   notify_xxx()
> if (vm->in_sbm)
> return
>   notify_xxx()
> if (vm->in_sbm)
> return
> 
> This style looks a little better to me.

Then we lose all the shared code after any of the mode-specific
handling? Like we have in MEM_OFFLINE, MEM_ONLINE, MEM_CANCEL_OFFLINE, ...

Don't think this will improve the situation.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 21/29] virtio-mem: memory notifier callbacks are specific to Sub Block Mode (SBM)

2020-10-18 Thread Wei Yang 
On Mon, Oct 12, 2020 at 02:53:15PM +0200, David Hildenbrand wrote:
>Let's rename accordingly.
>
>Cc: "Michael S. Tsirkin" 
>Cc: Jason Wang 
>Cc: Pankaj Gupta 
>Signed-off-by: David Hildenbrand 
>---
> drivers/virtio/virtio_mem.c | 29 +++--
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>index 3a772714fec9..d06c8760b337 100644
>--- a/drivers/virtio/virtio_mem.c
>+++ b/drivers/virtio/virtio_mem.c
>@@ -589,8 +589,8 @@ static bool virtio_mem_contains_range(struct virtio_mem 
>*vm, uint64_t start,
>   return start >= vm->addr && start + size <= vm->addr + vm->region_size;
> }
> 
>-static int virtio_mem_notify_going_online(struct virtio_mem *vm,
>-unsigned long mb_id)
>+static int virtio_mem_sbm_notify_going_online(struct virtio_mem *vm,
>+unsigned long mb_id)

Look into this patch with "virtio-mem: Big Block Mode (BBM) memory hotplug"
together, I thought the code is a little "complex".

The final logic of virtio_mem_memory_notifier_cb() looks like this:

virtio_mem_memory_notifier_cb()
if (vm->in_sbm)
notify_xxx()
if (vm->in_sbm)
notify_xxx()

Can we adjust this like

virtio_mem_memory_notifier_cb()
notify_xxx()
if (vm->in_sbm)
return
notify_xxx()
if (vm->in_sbm)
return

This style looks a little better to me.


-- 
Wei Yang
Help you, Help me

[PATCH v1 21/29] virtio-mem: memory notifier callbacks are specific to Sub Block Mode (SBM)

2020-10-12 Thread David Hildenbrand 
Let's rename accordingly.

Cc: "Michael S. Tsirkin" 
Cc: Jason Wang 
Cc: Pankaj Gupta 
Signed-off-by: David Hildenbrand 
---
 drivers/virtio/virtio_mem.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 3a772714fec9..d06c8760b337 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -589,8 +589,8 @@ static bool virtio_mem_contains_range(struct virtio_mem 
*vm, uint64_t start,
return start >= vm->addr && start + size <= vm->addr + vm->region_size;
 }
 
-static int virtio_mem_notify_going_online(struct virtio_mem *vm,
- unsigned long mb_id)
+static int virtio_mem_sbm_notify_going_online(struct virtio_mem *vm,
+ unsigned long mb_id)
 {
switch (virtio_mem_sbm_get_mb_state(vm, mb_id)) {
case VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL:
@@ -604,8 +604,8 @@ static int virtio_mem_notify_going_online(struct virtio_mem 
*vm,
return NOTIFY_BAD;
 }
 
-static void virtio_mem_notify_offline(struct virtio_mem *vm,
- unsigned long mb_id)
+static void virtio_mem_sbm_notify_offline(struct virtio_mem *vm,
+ unsigned long mb_id)
 {
switch (virtio_mem_sbm_get_mb_state(vm, mb_id)) {
case VIRTIO_MEM_SBM_MB_ONLINE_PARTIAL:
@@ -622,7 +622,8 @@ static void virtio_mem_notify_offline(struct virtio_mem *vm,
}
 }
 
-static void virtio_mem_notify_online(struct virtio_mem *vm, unsigned long 
mb_id)
+static void virtio_mem_sbm_notify_online(struct virtio_mem *vm,
+unsigned long mb_id)
 {
switch (virtio_mem_sbm_get_mb_state(vm, mb_id)) {
case VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL:
@@ -639,8 +640,8 @@ static void virtio_mem_notify_online(struct virtio_mem *vm, 
unsigned long mb_id)
}
 }
 
-static void virtio_mem_notify_going_offline(struct virtio_mem *vm,
-   unsigned long mb_id)
+static void virtio_mem_sbm_notify_going_offline(struct virtio_mem *vm,
+   unsigned long mb_id)
 {
const unsigned long nr_pages = PFN_DOWN(vm->sbm.sb_size);
unsigned long pfn;
@@ -655,8 +656,8 @@ static void virtio_mem_notify_going_offline(struct 
virtio_mem *vm,
}
 }
 
-static void virtio_mem_notify_cancel_offline(struct virtio_mem *vm,
-unsigned long mb_id)
+static void virtio_mem_sbm_notify_cancel_offline(struct virtio_mem *vm,
+unsigned long mb_id)
 {
const unsigned long nr_pages = PFN_DOWN(vm->sbm.sb_size);
unsigned long pfn;
@@ -716,7 +717,7 @@ static int virtio_mem_memory_notifier_cb(struct 
notifier_block *nb,
break;
}
vm->hotplug_active = true;
-   virtio_mem_notify_going_offline(vm, mb_id);
+   virtio_mem_sbm_notify_going_offline(vm, mb_id);
break;
case MEM_GOING_ONLINE:
mutex_lock(>hotplug_mutex);
@@ -726,10 +727,10 @@ static int virtio_mem_memory_notifier_cb(struct 
notifier_block *nb,
break;
}
vm->hotplug_active = true;
-   rc = virtio_mem_notify_going_online(vm, mb_id);
+   rc = virtio_mem_sbm_notify_going_online(vm, mb_id);
break;
case MEM_OFFLINE:
-   virtio_mem_notify_offline(vm, mb_id);
+   virtio_mem_sbm_notify_offline(vm, mb_id);
 
atomic64_add(size, >offline_size);
/*
@@ -743,7 +744,7 @@ static int virtio_mem_memory_notifier_cb(struct 
notifier_block *nb,
mutex_unlock(>hotplug_mutex);
break;
case MEM_ONLINE:
-   virtio_mem_notify_online(vm, mb_id);
+   virtio_mem_sbm_notify_online(vm, mb_id);
 
atomic64_sub(size, >offline_size);
/*
@@ -762,7 +763,7 @@ static int virtio_mem_memory_notifier_cb(struct 
notifier_block *nb,
case MEM_CANCEL_OFFLINE:
if (!vm->hotplug_active)
break;
-   virtio_mem_notify_cancel_offline(vm, mb_id);
+   virtio_mem_sbm_notify_cancel_offline(vm, mb_id);
vm->hotplug_active = false;
mutex_unlock(>hotplug_mutex);
break;
-- 
2.26.2