[Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Wei Yang]

When we iterate the memory-device list to get the available range, it is not
necessary to iterate the whole list.

1) no more overlap for hinted range if tmp exceed it

v2:
   * remove #2 as suggested by Igor and David
   * add some comment to inform address assignment stay the same as before
     this change 

Wei Yang (2):
  memory-device: not necessary to use goto for the last check
  memory-device: break the loop if tmp exceed the hinted range

 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

We are already at the last condition check.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 5f2c408036..df3261b32a 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -186,7 +186,6 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
-        goto out;
     }
 out:
     g_slist_free(list);
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range

[Wei Yang]

The memory-device list built by memory_device_build_list is ordered by
its address, this means if the tmp range exceed the hinted range, all
the following range will not overlap with it.

And this won't change default pc-dimm mapping and address assignment stay
the same as before this change.

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..df4e338b83 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 (range_lob(&tmp) > range_upb(&new)) {
+            break;
         }
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range

[David Hildenbrand]

On 30.07.19 02:37, Wei Yang wrote:
> The memory-device list built by memory_device_build_list is ordered by
> its address, this means if the tmp range exceed the hinted range, all
> the following range will not overlap with it.
> 
> And this won't change default pc-dimm mapping and address assignment stay
> the same as before this change.
> 
> 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..df4e338b83 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 (range_lob(&tmp) > range_upb(&new)) {
> +            break;
>          }
>      }
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 2/2] memory-device: break the loop if tmp exceed the hinted range

[Igor Mammedov]

On Tue, 30 Jul 2019 08:37:40 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> The memory-device list built by memory_device_build_list is ordered by
> its address, this means if the tmp range exceed the hinted range, all
> the following range will not overlap with it.
> 
> And this won't change default pc-dimm mapping and address assignment stay
> the same as before this change.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.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..df4e338b83 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 (range_lob(&tmp) > range_upb(&new)) {
> +            break;
>          }
>      }
>  

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Zeng, Star]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> Sent: Tuesday, July 30, 2019 8:38 AM
> To: qemu-devel@nongnu.org
> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> <richardw.yang@linux.intel.com>; mst@redhat.com
> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
> goto for the last check
> 
> We are already at the last condition check.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
> 5f2c408036..df3261b32a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -186,7 +186,6 @@ static uint64_t
> memory_device_get_free_addr(MachineState *ms,
>      if (!range_contains_range(&as, &new)) {
>          error_setg(errp, "could not find position in guest address space for "
>                     "memory device - memory fragmented due to alignments");
> -        goto out;

Is it better to return 0 (or set new_addr to 0) for this error path and another remaining "goto out" path?


Thanks,
Star

>      }
>  out:
>      g_slist_free(list);
> --
> 2.17.1
> 

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> -----Original Message-----
>> From: Qemu-devel [mailto:qemu-devel-
>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> Sent: Tuesday, July 30, 2019 8:38 AM
>> To: qemu-devel@nongnu.org
>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> <richardw.yang@linux.intel.com>; mst@redhat.com
>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
>> goto for the last check
>> 
>> We are already at the last condition check.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/memory-device.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
>> 5f2c408036..df3261b32a 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -186,7 +186,6 @@ static uint64_t
>> memory_device_get_free_addr(MachineState *ms,
>>      if (!range_contains_range(&as, &new)) {
>>          error_setg(errp, "could not find position in guest address space for "
>>                     "memory device - memory fragmented due to alignments");
>> -        goto out;
>
>Is it better to return 0 (or set new_addr to 0) for this error path and another remaining "goto out" path?
>

I may not get your point.

We set errp which is handled in its caller. By doing so, the error is
propagated.

Do I miss something?

>
>Thanks,
>Star
>
>>      }
>>  out:
>>      g_slist_free(list);
>> --
>> 2.17.1
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Zeng, Star]

> -----Original Message-----
> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> Sent: Thursday, August 8, 2019 10:13 AM
> To: Zeng, Star <star.zeng@intel.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
> imammedo@redhat.com; david@redhat.com; mst@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
> 
> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
> >> -----Original Message-----
> >> From: Qemu-devel [mailto:qemu-devel-
> >> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> >> Sent: Tuesday, July 30, 2019 8:38 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> >> <richardw.yang@linux.intel.com>; mst@redhat.com
> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> >> use goto for the last check
> >>
> >> We are already at the last condition check.
> >>
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >> Reviewed-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/mem/memory-device.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index
> >> 5f2c408036..df3261b32a 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -186,7 +186,6 @@ static uint64_t
> >> memory_device_get_free_addr(MachineState *ms,
> >>      if (!range_contains_range(&as, &new)) {
> >>          error_setg(errp, "could not find position in guest address space for "
> >>                     "memory device - memory fragmented due to alignments");
> >> -        goto out;
> >
> >Is it better to return 0 (or set new_addr to 0) for this error path and another
> remaining "goto out" path?
> >
> 
> I may not get your point.
> 
> We set errp which is handled in its caller. By doing so, the error is propagated.
> 
> Do I miss something?

Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)

Thanks,
Star

> 
> >
> >Thanks,
> >Star
> >
> >>      }
> >>  out:
> >>      g_slist_free(list);
> >> --
> >> 2.17.1
> >>
> 
> --
> Wei Yang
> Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>> -----Original Message-----
>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> Sent: Thursday, August 8, 2019 10:13 AM
>> To: Zeng, Star <star.zeng@intel.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> use goto for the last check
>> 
>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> >> -----Original Message-----
>> >> From: Qemu-devel [mailto:qemu-devel-
>> >> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> >> Sent: Tuesday, July 30, 2019 8:38 AM
>> >> To: qemu-devel@nongnu.org
>> >> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> >> <richardw.yang@linux.intel.com>; mst@redhat.com
>> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> >> use goto for the last check
>> >>
>> >> We are already at the last condition check.
>> >>
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> >> Reviewed-by: David Hildenbrand <david@redhat.com>
>> >> ---
>> >>  hw/mem/memory-device.c | 1 -
>> >>  1 file changed, 1 deletion(-)
>> >>
>> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index
>> >> 5f2c408036..df3261b32a 100644
>> >> --- a/hw/mem/memory-device.c
>> >> +++ b/hw/mem/memory-device.c
>> >> @@ -186,7 +186,6 @@ static uint64_t
>> >> memory_device_get_free_addr(MachineState *ms,
>> >>      if (!range_contains_range(&as, &new)) {
>> >>          error_setg(errp, "could not find position in guest address space for "
>> >>                     "memory device - memory fragmented due to alignments");
>> >> -        goto out;
>> >
>> >Is it better to return 0 (or set new_addr to 0) for this error path and another
>> remaining "goto out" path?
>> >
>> 
>> I may not get your point.
>> 
>> We set errp which is handled in its caller. By doing so, the error is propagated.
>> 
>> Do I miss something?
>
>Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>

You may have some point.

Let's see whether others have the same taste, or we can refine it separately.

>Thanks,
>Star
>
>> 
>> >
>> >Thanks,
>> >Star
>> >
>> >>      }
>> >>  out:
>> >>      g_slist_free(list);
>> >> --
>> >> 2.17.1
>> >>
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[David Hildenbrand]

On 08.08.19 04:38, Wei Yang wrote:
> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>>> -----Original Message-----
>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>>> Sent: Thursday, August 8, 2019 10:13 AM
>>> To: Zeng, Star <star.zeng@intel.com>
>>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>> use goto for the last check
>>>
>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>>>>> -----Original Message-----
>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>>> use goto for the last check
>>>>>
>>>>> We are already at the last condition check.
>>>>>
>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/mem/memory-device.c | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>> index
>>>>> 5f2c408036..df3261b32a 100644
>>>>> --- a/hw/mem/memory-device.c
>>>>> +++ b/hw/mem/memory-device.c
>>>>> @@ -186,7 +186,6 @@ static uint64_t
>>>>> memory_device_get_free_addr(MachineState *ms,
>>>>>      if (!range_contains_range(&as, &new)) {
>>>>>          error_setg(errp, "could not find position in guest address space for "
>>>>>                     "memory device - memory fragmented due to alignments");
>>>>> -        goto out;
>>>>
>>>> Is it better to return 0 (or set new_addr to 0) for this error path and another
>>> remaining "goto out" path?
>>>>
>>>
>>> I may not get your point.
>>>
>>> We set errp which is handled in its caller. By doing so, the error is propagated.
>>>
>>> Do I miss something?
>>
>> Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>> But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>>
> 
> You may have some point.
> 
> Let's see whether others have the same taste, or we can refine it separately.
> 

I don't think that's necessary (callers really should check for errors
before using the return values), but I would also not object to that change.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
>On 08.08.19 04:38, Wei Yang wrote:
>> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>>>> -----Original Message-----
>>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>>>> Sent: Thursday, August 8, 2019 10:13 AM
>>>> To: Zeng, Star <star.zeng@intel.com>
>>>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>>>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>> use goto for the last check
>>>>
>>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>>>>>> -----Original Message-----
>>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>>> To: qemu-devel@nongnu.org
>>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>>>> use goto for the last check
>>>>>>
>>>>>> We are already at the last condition check.
>>>>>>
>>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>>  hw/mem/memory-device.c | 1 -
>>>>>>  1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>>> index
>>>>>> 5f2c408036..df3261b32a 100644
>>>>>> --- a/hw/mem/memory-device.c
>>>>>> +++ b/hw/mem/memory-device.c
>>>>>> @@ -186,7 +186,6 @@ static uint64_t
>>>>>> memory_device_get_free_addr(MachineState *ms,
>>>>>>      if (!range_contains_range(&as, &new)) {
>>>>>>          error_setg(errp, "could not find position in guest address space for "
>>>>>>                     "memory device - memory fragmented due to alignments");
>>>>>> -        goto out;
>>>>>
>>>>> Is it better to return 0 (or set new_addr to 0) for this error path and another
>>>> remaining "goto out" path?
>>>>>
>>>>
>>>> I may not get your point.
>>>>
>>>> We set errp which is handled in its caller. By doing so, the error is propagated.
>>>>
>>>> Do I miss something?
>>>
>>> Yes, you are right. Currently, the caller is checking errp, but not the returned address, so there should be no issue.
>>> But when you see other error paths, you will find they all return 0. To be aligned (return 0 when error), so just suggest also returning 0 for these two "goto out" error path. :)
>>>
>> 
>> You may have some point.
>> 
>> Let's see whether others have the same taste, or we can refine it separately.
>> 
>
>I don't think that's necessary (callers really should check for errors
>before using the return values), but I would also not object to that change.
>

In case there is no strong requirement to refactor the code. I would leave it
here.

>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Zeng, Star]

> -----Original Message-----
> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> Sent: Monday, August 19, 2019 10:39 AM
> To: David Hildenbrand <david@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
> mst@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
> 
> On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
> >On 08.08.19 04:38, Wei Yang wrote:
> >> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
> >>>> -----Original Message-----
> >>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> >>>> Sent: Thursday, August 8, 2019 10:13 AM
> >>>> To: Zeng, Star <star.zeng@intel.com>
> >>>> Cc: Wei Yang <richardw.yang@linux.intel.com>;
> >>>> qemu-devel@nongnu.org; imammedo@redhat.com;
> david@redhat.com;
> >>>> mst@redhat.com
> >>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> >>>> necessary to use goto for the last check
> >>>>
> >>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Qemu-devel [mailto:qemu-devel-
> >>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
> >>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
> >>>>>> To: qemu-devel@nongnu.org
> >>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
> >>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
> >>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> necessary
> >>>>>> to use goto for the last check
> >>>>>>
> >>>>>> We are already at the last condition check.
> >>>>>>
> >>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
> >>>>>> ---
> >>>>>>  hw/mem/memory-device.c | 1 -
> >>>>>>  1 file changed, 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-
> device.c
> >>>> index
> >>>>>> 5f2c408036..df3261b32a 100644
> >>>>>> --- a/hw/mem/memory-device.c
> >>>>>> +++ b/hw/mem/memory-device.c
> >>>>>> @@ -186,7 +186,6 @@ static uint64_t
> >>>>>> memory_device_get_free_addr(MachineState *ms,
> >>>>>>      if (!range_contains_range(&as, &new)) {
> >>>>>>          error_setg(errp, "could not find position in guest address space
> for "
> >>>>>>                     "memory device - memory fragmented due to
> alignments");
> >>>>>> -        goto out;
> >>>>>
> >>>>> Is it better to return 0 (or set new_addr to 0) for this error
> >>>>> path and another
> >>>> remaining "goto out" path?
> >>>>>
> >>>>
> >>>> I may not get your point.
> >>>>
> >>>> We set errp which is handled in its caller. By doing so, the error is
> propagated.
> >>>>
> >>>> Do I miss something?
> >>>
> >>> Yes, you are right. Currently, the caller is checking errp, but not the
> returned address, so there should be no issue.
> >>> But when you see other error paths, you will find they all return 0.
> >>> To be aligned (return 0 when error), so just suggest also returning
> >>> 0 for these two "goto out" error path. :)
> >>>
> >>
> >> You may have some point.
> >>
> >> Let's see whether others have the same taste, or we can refine it
> separately.
> >>
> >
> >I don't think that's necessary (callers really should check for errors
> >before using the return values), but I would also not object to that change.
> >
> 
> In case there is no strong requirement to refactor the code. I would leave it
> here.

It was just my suggestion. I am fine with any preference you and other experts have.

Thanks,
Star

> 
> >--
> >
> >Thanks,
> >
> >David / dhildenb
> 
> --
> Wei Yang
> Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[Wei Yang]

On Mon, Aug 19, 2019 at 05:32:14AM +0000, Zeng, Star wrote:
>
>
>> -----Original Message-----
>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> Sent: Monday, August 19, 2019 10:39 AM
>> To: David Hildenbrand <david@redhat.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
>> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
>> mst@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> use goto for the last check
>> 
>> On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
>> >On 08.08.19 04:38, Wei Yang wrote:
>> >> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>> >>>> -----Original Message-----
>> >>>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> >>>> Sent: Thursday, August 8, 2019 10:13 AM
>> >>>> To: Zeng, Star <star.zeng@intel.com>
>> >>>> Cc: Wei Yang <richardw.yang@linux.intel.com>;
>> >>>> qemu-devel@nongnu.org; imammedo@redhat.com;
>> david@redhat.com;
>> >>>> mst@redhat.com
>> >>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not
>> >>>> necessary to use goto for the last check
>> >>>>
>> >>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>> >>>>>> -----Original Message-----
>> >>>>>> From: Qemu-devel [mailto:qemu-devel-
>> >>>>>> bounces+star.zeng=intel.com@nongnu.org] On Behalf Of Wei Yang
>> >>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>> >>>>>> To: qemu-devel@nongnu.org
>> >>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>> >>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>> >>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not
>> necessary
>> >>>>>> to use goto for the last check
>> >>>>>>
>> >>>>>> We are already at the last condition check.
>> >>>>>>
>> >>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> >>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> >>>>>> ---
>> >>>>>>  hw/mem/memory-device.c | 1 -
>> >>>>>>  1 file changed, 1 deletion(-)
>> >>>>>>
>> >>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-
>> device.c
>> >>>> index
>> >>>>>> 5f2c408036..df3261b32a 100644
>> >>>>>> --- a/hw/mem/memory-device.c
>> >>>>>> +++ b/hw/mem/memory-device.c
>> >>>>>> @@ -186,7 +186,6 @@ static uint64_t
>> >>>>>> memory_device_get_free_addr(MachineState *ms,
>> >>>>>>      if (!range_contains_range(&as, &new)) {
>> >>>>>>          error_setg(errp, "could not find position in guest address space
>> for "
>> >>>>>>                     "memory device - memory fragmented due to
>> alignments");
>> >>>>>> -        goto out;
>> >>>>>
>> >>>>> Is it better to return 0 (or set new_addr to 0) for this error
>> >>>>> path and another
>> >>>> remaining "goto out" path?
>> >>>>>
>> >>>>
>> >>>> I may not get your point.
>> >>>>
>> >>>> We set errp which is handled in its caller. By doing so, the error is
>> propagated.
>> >>>>
>> >>>> Do I miss something?
>> >>>
>> >>> Yes, you are right. Currently, the caller is checking errp, but not the
>> returned address, so there should be no issue.
>> >>> But when you see other error paths, you will find they all return 0.
>> >>> To be aligned (return 0 when error), so just suggest also returning
>> >>> 0 for these two "goto out" error path. :)
>> >>>
>> >>
>> >> You may have some point.
>> >>
>> >> Let's see whether others have the same taste, or we can refine it
>> separately.
>> >>
>> >
>> >I don't think that's necessary (callers really should check for errors
>> >before using the return values), but I would also not object to that change.
>> >
>> 
>> In case there is no strong requirement to refactor the code. I would leave it
>> here.
>
>It was just my suggestion. I am fine with any preference you and other experts have.
>

Thanks

>Thanks,
>Star
>
>> 
>> >--
>> >
>> >Thanks,
>> >
>> >David / dhildenb
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Wei Yang]

On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
>When we iterate the memory-device list to get the available range, it is not
>necessary to iterate the whole list.
>
>1) no more overlap for hinted range if tmp exceed it
>
>v2:
>   * remove #2 as suggested by Igor and David
>   * add some comment to inform address assignment stay the same as before
>     this change 
>
>Wei Yang (2):
>  memory-device: not necessary to use goto for the last check
>  memory-device: break the loop if tmp exceed the hinted range
>
> hw/mem/memory-device.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>

Would someone take this patch set?

>-- 
>2.17.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Michael S. Tsirkin]

On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote:
> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
> >When we iterate the memory-device list to get the available range, it is not
> >necessary to iterate the whole list.
> >
> >1) no more overlap for hinted range if tmp exceed it
> >
> >v2:
> >   * remove #2 as suggested by Igor and David
> >   * add some comment to inform address assignment stay the same as before
> >     this change 
> >
> >Wei Yang (2):
> >  memory-device: not necessary to use goto for the last check
> >  memory-device: break the loop if tmp exceed the hinted range
> >
> > hw/mem/memory-device.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> 
> Would someone take this patch set?

yes looks good to me too.
Eduardo?

> >-- 
> >2.17.1
> >
> 
> -- 
> Wei Yang
> Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Wei Yang]

On Sat, Sep 14, 2019 at 03:40:41PM -0400, Michael S. Tsirkin wrote:
>On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote:
>> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
>> >When we iterate the memory-device list to get the available range, it is not
>> >necessary to iterate the whole list.
>> >
>> >1) no more overlap for hinted range if tmp exceed it
>> >
>> >v2:
>> >   * remove #2 as suggested by Igor and David
>> >   * add some comment to inform address assignment stay the same as before
>> >     this change 
>> >
>> >Wei Yang (2):
>> >  memory-device: not necessary to use goto for the last check
>> >  memory-device: break the loop if tmp exceed the hinted range
>> >
>> > hw/mem/memory-device.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> 
>> Would someone take this patch set?
>
>yes looks good to me too.
>Eduardo?
>

Hmm... I don't see this any where. May I ask the status?

>> >-- 
>> >2.17.1
>> >
>> 
>> -- 
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Eduardo Habkost]

On Sat, Oct 12, 2019 at 05:02:09PM +0800, Wei Yang wrote:
> On Sat, Sep 14, 2019 at 03:40:41PM -0400, Michael S. Tsirkin wrote:
> >On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote:
> >> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
> >> >When we iterate the memory-device list to get the available range, it is not
> >> >necessary to iterate the whole list.
> >> >
> >> >1) no more overlap for hinted range if tmp exceed it
> >> >
> >> >v2:
> >> >   * remove #2 as suggested by Igor and David
> >> >   * add some comment to inform address assignment stay the same as before
> >> >     this change 
> >> >
> >> >Wei Yang (2):
> >> >  memory-device: not necessary to use goto for the last check
> >> >  memory-device: break the loop if tmp exceed the hinted range
> >> >
> >> > hw/mem/memory-device.c | 3 ++-
> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> 
> >> Would someone take this patch set?
> >
> >yes looks good to me too.
> >Eduardo?
> >
> 
> Hmm... I don't see this any where. May I ask the status?

Sorry, I hadn't seen Michael's message.  Queued on machine-next.
Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr

[Wei Yang]

On Mon, Oct 14, 2019 at 12:05:47PM -0300, Eduardo Habkost wrote:
>On Sat, Oct 12, 2019 at 05:02:09PM +0800, Wei Yang wrote:
>> On Sat, Sep 14, 2019 at 03:40:41PM -0400, Michael S. Tsirkin wrote:
>> >On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote:
>> >> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote:
>> >> >When we iterate the memory-device list to get the available range, it is not
>> >> >necessary to iterate the whole list.
>> >> >
>> >> >1) no more overlap for hinted range if tmp exceed it
>> >> >
>> >> >v2:
>> >> >   * remove #2 as suggested by Igor and David
>> >> >   * add some comment to inform address assignment stay the same as before
>> >> >     this change 
>> >> >
>> >> >Wei Yang (2):
>> >> >  memory-device: not necessary to use goto for the last check
>> >> >  memory-device: break the loop if tmp exceed the hinted range
>> >> >
>> >> > hw/mem/memory-device.c | 3 ++-
>> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >> >
>> >> 
>> >> Would someone take this patch set?
>> >
>> >yes looks good to me too.
>> >Eduardo?
>> >
>> 
>> Hmm... I don't see this any where. May I ask the status?
>
>Sorry, I hadn't seen Michael's message.  Queued on machine-next.
>Thanks!
>

Thanks~ have a nice day.

>-- 
>Eduardo

-- 
Wei Yang
Help you, Help me