| Message ID | 20190728131304.1282-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: refine memory_device_get_free_addr | expand |
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; > } > } >
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>
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; >> } >> } >>
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
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; } }
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(+)