[2/3] memory-device: break the loop if no hint is provided
diff mbox series

Message ID 20190728131304.1282-3-richardw.yang@linux.intel.com
State New
Headers show
Series
  • memory-device: refine memory_device_get_free_addr
Related show

Commit Message

Wei Yang July 28, 2019, 1:13 p.m. UTC
When there is no hint, the first un-overlapped range is the proper one.
Just break the loop instead of iterate the whole list.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/mem/memory-device.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Igor Mammedov July 29, 2019, 7:04 a.m. UTC | #1
On Sun, 28 Jul 2019 21:13:03 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> When there is no hint, the first un-overlapped range is the proper one.
> Just break the loop instead of iterate the whole list.
could it change default pc-dimm mapping (will address assignment stay
the same as before this change)?

In commit message I'd suggest to replace 'the proper one' with more
verbose explanation why it is safe to break earlier.

otherwise patch look good to me

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/mem/memory-device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index df3261b32a..413b514586 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>                  range_make_empty(&new);
>                  break;
>              }
> +        } else if (!hint) {
> +            break;
>          }
>      }
>
David Hildenbrand July 29, 2019, 7:45 a.m. UTC | #2
On 28.07.19 15:13, Wei Yang wrote:
> When there is no hint, the first un-overlapped range is the proper one.
> Just break the loop instead of iterate the whole list.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/mem/memory-device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index df3261b32a..413b514586 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>                  range_make_empty(&new);
>                  break;
>              }
> +        } else if (!hint) {
> +            break;
>          }
>      }
>  
> 

I think

a) This is fine. I was not able to construct a counter-example where
this would not work. Whenever we modify the range, we check against the
next one in the sorted list. If there is no overlap, it fits. And, it
won't overlap with any other range (and therefore never be changed again)

b) This should therefore not change the assignment order / break migration.

Maybe mention that this will not change the assigned addresses compared
to old code in all scenarios.

Reviewed-by: David Hildenbrand <david@redhat.com>
Wei Yang July 29, 2019, 7:49 a.m. UTC | #3
On Mon, Jul 29, 2019 at 09:04:01AM +0200, Igor Mammedov wrote:
>On Sun, 28 Jul 2019 21:13:03 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> When there is no hint, the first un-overlapped range is the proper one.
>> Just break the loop instead of iterate the whole list.
>could it change default pc-dimm mapping (will address assignment stay
>the same as before this change)?

Yes, it stays the same as before.

>
>In commit message I'd suggest to replace 'the proper one' with more
>verbose explanation why it is safe to break earlier.
>

ok, let me re-phrase it. Thanks

>otherwise patch look good to me
>
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/mem/memory-device.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index df3261b32a..413b514586 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>                  range_make_empty(&new);
>>                  break;
>>              }
>> +        } else if (!hint) {
>> +            break;
>>          }
>>      }
>>
Wei Yang July 29, 2019, 7:50 a.m. UTC | #4
On Mon, Jul 29, 2019 at 09:45:24AM +0200, David Hildenbrand wrote:
>On 28.07.19 15:13, Wei Yang wrote:
>> When there is no hint, the first un-overlapped range is the proper one.
>> Just break the loop instead of iterate the whole list.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/mem/memory-device.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index df3261b32a..413b514586 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -180,6 +180,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>                  range_make_empty(&new);
>>                  break;
>>              }
>> +        } else if (!hint) {
>> +            break;
>>          }
>>      }
>>  
>> 
>
>I think
>
>a) This is fine. I was not able to construct a counter-example where
>this would not work. Whenever we modify the range, we check against the
>next one in the sorted list. If there is no overlap, it fits. And, it
>won't overlap with any other range (and therefore never be changed again)
>
>b) This should therefore not change the assignment order / break migration.
>
>Maybe mention that this will not change the assigned addresses compared
>to old code in all scenarios.
>

Thanks, let me add this in change log.

>Reviewed-by: David Hildenbrand <david@redhat.com>
>
>-- 
>
>Thanks,
>
>David / dhildenb

Patch
diff mbox series

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index df3261b32a..413b514586 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -180,6 +180,8 @@  static uint64_t memory_device_get_free_addr(MachineState *ms,
                 range_make_empty(&new);
                 break;
             }
+        } else if (!hint) {
+            break;
         }
     }