Re: [PATCH v2 2/6] accel: accel_available() function
Claudio Fontana writes:
> On 11/27/20 3:45 PM, Markus Armbruster wrote:
>> Claudio Fontana writes:
>>
>>> On 11/26/20 10:48 PM, Eduardo Habkost wrote:
On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
> On 11/26/20 3:25 PM, Paolo Bonzini wrote:
>> On 26/11/20 15:13, Claudio Fontana wrote:
>>> One option I see is simply to document the behavior where
>>> accel_available() is declared in accel.h (ie do not use in fast
>>> path), as well as in accel_find() actually, so that both accel_find()
>>> and accel_available() are avoided in fast path and avoid being called
>>> frequently at runtime.
>>>
>>> Another option could be to remove the allocation completely, and use
>>> for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
>>> again would be to remove the allocation and use either a fixed buffer
>>> + snprintf, or alloca -like builtin code to use the stack, ...
>>>
>>> Not a big deal, but with a general utility and short name like
>>> accel_available(name) it might be tempting to use this more in the
>>> future?
>>
>> I think it's just that the usecase is not that common. "Is this
>> accelerator compiled in the binary" is not something you need after
>> startup (or if querying the monitor).
>>
>> Paolo
>>
>>
>
> A script that repeatedly uses the QMP interface to query for
> the status could generate fragmentation this way I think.
Is this a problem? Today, execution of a "query-kvm" command
calls g_malloc() 37 times.
>>>
>>> Not ideal in my view, but not the end of the world either.
>>
>> QMP's appetite for malloc is roughly comparable to a pig's for truffles.
>>
>
> :-)
>
> Btw, do we have limits on the maximum size of these objects? I mean, a single
> QMP command,
> a single QEMU object type name, etc?
>
> In this case we could do some overall improvement there, and might even avoid
> some problems down the road..
We have limits, but they are not comprehensive.
The QMP client is trusted. We don't try to guard against a malicious
QMP client. We do try to guard against mistakes.
The JSON parser limits token size (in characters), expression size (in
tokens), and expression nesting depth. This protects against a
malfunctioning QMP client. The limits are ridiculously generous.
The QMP core limits the number of commands in flight per monitor to a
somewhat parsimonious 8-9 in-band commands, plus one out-of-band
command. This protects against a QMP client sending commands faster
than we can execute them.
QMP output is buffered without limit. When a (malfunctioning) QMP
client keeps sending commands without reading their output, QEMU keeps
buffering until it runs out of memory and crashes.
Re: [PATCH v2 2/6] accel: accel_available() function
On 11/27/20 3:45 PM, Markus Armbruster wrote:
> Claudio Fontana writes:
>
>> On 11/26/20 10:48 PM, Eduardo Habkost wrote:
>>> On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
On 11/26/20 3:25 PM, Paolo Bonzini wrote:
> On 26/11/20 15:13, Claudio Fontana wrote:
>> One option I see is simply to document the behavior where
>> accel_available() is declared in accel.h (ie do not use in fast
>> path), as well as in accel_find() actually, so that both accel_find()
>> and accel_available() are avoided in fast path and avoid being called
>> frequently at runtime.
>>
>> Another option could be to remove the allocation completely, and use
>> for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
>> again would be to remove the allocation and use either a fixed buffer
>> + snprintf, or alloca -like builtin code to use the stack, ...
>>
>> Not a big deal, but with a general utility and short name like
>> accel_available(name) it might be tempting to use this more in the
>> future?
>
> I think it's just that the usecase is not that common. "Is this
> accelerator compiled in the binary" is not something you need after
> startup (or if querying the monitor).
>
> Paolo
>
>
A script that repeatedly uses the QMP interface to query for
the status could generate fragmentation this way I think.
>>>
>>> Is this a problem? Today, execution of a "query-kvm" command
>>> calls g_malloc() 37 times.
>>>
>>
>> Not ideal in my view, but not the end of the world either.
>
> QMP's appetite for malloc is roughly comparable to a pig's for truffles.
>
:-)
Btw, do we have limits on the maximum size of these objects? I mean, a single
QMP command,
a single QEMU object type name, etc?
In this case we could do some overall improvement there, and might even avoid
some problems down the road..
Ciao,
Claudio
Re: [PATCH v2 2/6] accel: accel_available() function
Claudio Fontana writes:
> On 11/26/20 10:48 PM, Eduardo Habkost wrote:
>> On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
>>> On 11/26/20 3:25 PM, Paolo Bonzini wrote:
On 26/11/20 15:13, Claudio Fontana wrote:
> One option I see is simply to document the behavior where
> accel_available() is declared in accel.h (ie do not use in fast
> path), as well as in accel_find() actually, so that both accel_find()
> and accel_available() are avoided in fast path and avoid being called
> frequently at runtime.
>
> Another option could be to remove the allocation completely, and use
> for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
> again would be to remove the allocation and use either a fixed buffer
> + snprintf, or alloca -like builtin code to use the stack, ...
>
> Not a big deal, but with a general utility and short name like
> accel_available(name) it might be tempting to use this more in the
> future?
I think it's just that the usecase is not that common. "Is this
accelerator compiled in the binary" is not something you need after
startup (or if querying the monitor).
Paolo
>>>
>>> A script that repeatedly uses the QMP interface to query for
>>> the status could generate fragmentation this way I think.
>>
>> Is this a problem? Today, execution of a "query-kvm" command
>> calls g_malloc() 37 times.
>>
>
> Not ideal in my view, but not the end of the world either.
QMP's appetite for malloc is roughly comparable to a pig's for truffles.
Re: [PATCH v2 2/6] accel: accel_available() function
On Wed, 25 Nov 2020 15:56:32 -0500 Eduardo Habkost wrote: > This function will be used to replace the xen_available() and > kvm_available() functions from arch_init.c. > > Signed-off-by: Eduardo Habkost > --- > Cc: Richard Henderson > Cc: Paolo Bonzini > Cc: Claudio Fontana > Cc: Roman Bolshakov > --- > include/sysemu/accel.h | 1 + > accel/accel.c | 5 + > 2 files changed, 6 insertions(+) Reviewed-by: Cornelia Huck
Re: [PATCH v2 2/6] accel: accel_available() function
On 11/26/20 10:48 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
>> On 11/26/20 3:25 PM, Paolo Bonzini wrote:
>>> On 26/11/20 15:13, Claudio Fontana wrote:
One option I see is simply to document the behavior where
accel_available() is declared in accel.h (ie do not use in fast
path), as well as in accel_find() actually, so that both accel_find()
and accel_available() are avoided in fast path and avoid being called
frequently at runtime.
Another option could be to remove the allocation completely, and use
for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
again would be to remove the allocation and use either a fixed buffer
+ snprintf, or alloca -like builtin code to use the stack, ...
Not a big deal, but with a general utility and short name like
accel_available(name) it might be tempting to use this more in the
future?
>>>
>>> I think it's just that the usecase is not that common. "Is this
>>> accelerator compiled in the binary" is not something you need after
>>> startup (or if querying the monitor).
>>>
>>> Paolo
>>>
>>>
>>
>> A script that repeatedly uses the QMP interface to query for
>> the status could generate fragmentation this way I think.
>
> Is this a problem? Today, execution of a "query-kvm" command
> calls g_malloc() 37 times.
>
Not ideal in my view, but not the end of the world either.
Ciao,
Claudio
Re: [PATCH v2 2/6] accel: accel_available() function
On 26/11/20 22:06, Claudio Fontana wrote: I think it's just that the usecase is not that common. "Is this accelerator compiled in the binary" is not something you need after startup (or if querying the monitor). A script that repeatedly uses the QMP interface to query for the status could generate fragmentation this way I think. System malloc should be smarter than that, but anyway this is all but the only allocation in that path. Paolo
Re: [PATCH v2 2/6] accel: accel_available() function
On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
> On 11/26/20 3:25 PM, Paolo Bonzini wrote:
> > On 26/11/20 15:13, Claudio Fontana wrote:
> >> One option I see is simply to document the behavior where
> >> accel_available() is declared in accel.h (ie do not use in fast
> >> path), as well as in accel_find() actually, so that both accel_find()
> >> and accel_available() are avoided in fast path and avoid being called
> >> frequently at runtime.
> >>
> >> Another option could be to remove the allocation completely, and use
> >> for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
> >> again would be to remove the allocation and use either a fixed buffer
> >> + snprintf, or alloca -like builtin code to use the stack, ...
> >>
> >> Not a big deal, but with a general utility and short name like
> >> accel_available(name) it might be tempting to use this more in the
> >> future?
> >
> > I think it's just that the usecase is not that common. "Is this
> > accelerator compiled in the binary" is not something you need after
> > startup (or if querying the monitor).
> >
> > Paolo
> >
> >
>
> A script that repeatedly uses the QMP interface to query for
> the status could generate fragmentation this way I think.
Is this a problem? Today, execution of a "query-kvm" command
calls g_malloc() 37 times.
--
Eduardo
Re: [PATCH v2 2/6] accel: accel_available() function
On 11/26/20 3:25 PM, Paolo Bonzini wrote:
> On 26/11/20 15:13, Claudio Fontana wrote:
>> One option I see is simply to document the behavior where
>> accel_available() is declared in accel.h (ie do not use in fast
>> path), as well as in accel_find() actually, so that both accel_find()
>> and accel_available() are avoided in fast path and avoid being called
>> frequently at runtime.
>>
>> Another option could be to remove the allocation completely, and use
>> for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
>> again would be to remove the allocation and use either a fixed buffer
>> + snprintf, or alloca -like builtin code to use the stack, ...
>>
>> Not a big deal, but with a general utility and short name like
>> accel_available(name) it might be tempting to use this more in the
>> future?
>
> I think it's just that the usecase is not that common. "Is this
> accelerator compiled in the binary" is not something you need after
> startup (or if querying the monitor).
>
> Paolo
>
>
A script that repeatedly uses the QMP interface to query for the status could
generate fragmentation this way I think.
Ciao,
Claudio
Re: [PATCH v2 2/6] accel: accel_available() function
On 26/11/20 15:13, Claudio Fontana wrote:
One option I see is simply to document the behavior where
accel_available() is declared in accel.h (ie do not use in fast
path), as well as in accel_find() actually, so that both accel_find()
and accel_available() are avoided in fast path and avoid being called
frequently at runtime.
Another option could be to remove the allocation completely, and use
for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
again would be to remove the allocation and use either a fixed buffer
+ snprintf, or alloca -like builtin code to use the stack, ...
Not a big deal, but with a general utility and short name like
accel_available(name) it might be tempting to use this more in the
future?
I think it's just that the usecase is not that common. "Is this
accelerator compiled in the binary" is not something you need after
startup (or if querying the monitor).
Paolo
Re: [PATCH v2 2/6] accel: accel_available() function
On 11/26/20 2:36 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 10:14:31AM +0100, Claudio Fontana wrote:
>> Hi Eduardo,
>>
>> On 11/25/20 9:56 PM, Eduardo Habkost wrote:
>>> This function will be used to replace the xen_available() and
>>> kvm_available() functions from arch_init.c.
>>>
>>> Signed-off-by: Eduardo Habkost
>>> ---
>>> Cc: Richard Henderson
>>> Cc: Paolo Bonzini
>>> Cc: Claudio Fontana
>>> Cc: Roman Bolshakov
>>> ---
>>> include/sysemu/accel.h | 1 +
>>> accel/accel.c | 5 +
>>> 2 files changed, 6 insertions(+)
>>>
>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>> index e08b8ab8fa..a4a00c75c8 100644
>>> --- a/include/sysemu/accel.h
>>> +++ b/include/sysemu/accel.h
>>> @@ -67,6 +67,7 @@ typedef struct AccelClass {
>>> OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
>>>
>>> AccelClass *accel_find(const char *opt_name);
>>> +bool accel_available(const char *name);
>>> int accel_init_machine(AccelState *accel, MachineState *ms);
>>>
>>> /* Called just before os_setup_post (ie just before drop OS privs) */
>>> diff --git a/accel/accel.c b/accel/accel.c
>>> index cb555e3b06..4a64a2b38a 100644
>>> --- a/accel/accel.c
>>> +++ b/accel/accel.c
>>> @@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
>>> return ac;
>>> }
>>>
>>> +bool accel_available(const char *name)
>>> +{
>>> +return accel_find(name) != NULL;
>>
>>
>> accel_find() in its implementation allocates and then frees memory to
>> generate the string,
>> the user of accel_available() might be unaware and overuse leading to
>> fragmentation/performance issues?
>
> Is that a real issue? We had only 3 users of kvm_available() and
> xen_available() since those functions were added 10 years ago.
>
> Do you have any suggestions on what we should do?
>
One option I see is simply to document the behavior where accel_available() is
declared in accel.h (ie do not use in fast path), as well as in accel_find()
actually,
so that both accel_find() and accel_available() are avoided in fast path and
avoid being called frequently at runtime.
Another option could be to remove the allocation completely, and use for
example accel_find(ACCEL_CLASS_NAME("tcg")),
or another option again would be to remove the allocation and use either a
fixed buffer + snprintf, or alloca -like builtin code to use the stack, ...
Not a big deal, but with a general utility and short name like
accel_available(name) it might be tempting to use this more in the future?
Ciao,
Claudio
Re: [PATCH v2 2/6] accel: accel_available() function
On Thu, Nov 26, 2020 at 10:14:31AM +0100, Claudio Fontana wrote:
> Hi Eduardo,
>
> On 11/25/20 9:56 PM, Eduardo Habkost wrote:
> > This function will be used to replace the xen_available() and
> > kvm_available() functions from arch_init.c.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > Cc: Richard Henderson
> > Cc: Paolo Bonzini
> > Cc: Claudio Fontana
> > Cc: Roman Bolshakov
> > ---
> > include/sysemu/accel.h | 1 +
> > accel/accel.c | 5 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> > index e08b8ab8fa..a4a00c75c8 100644
> > --- a/include/sysemu/accel.h
> > +++ b/include/sysemu/accel.h
> > @@ -67,6 +67,7 @@ typedef struct AccelClass {
> > OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
> >
> > AccelClass *accel_find(const char *opt_name);
> > +bool accel_available(const char *name);
> > int accel_init_machine(AccelState *accel, MachineState *ms);
> >
> > /* Called just before os_setup_post (ie just before drop OS privs) */
> > diff --git a/accel/accel.c b/accel/accel.c
> > index cb555e3b06..4a64a2b38a 100644
> > --- a/accel/accel.c
> > +++ b/accel/accel.c
> > @@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
> > return ac;
> > }
> >
> > +bool accel_available(const char *name)
> > +{
> > +return accel_find(name) != NULL;
>
>
> accel_find() in its implementation allocates and then frees memory to
> generate the string,
> the user of accel_available() might be unaware and overuse leading to
> fragmentation/performance issues?
Is that a real issue? We had only 3 users of kvm_available() and
xen_available() since those functions were added 10 years ago.
Do you have any suggestions on what we should do?
--
Eduardo
Re: [PATCH v2 2/6] accel: accel_available() function
Hi Eduardo,
On 11/25/20 9:56 PM, Eduardo Habkost wrote:
> This function will be used to replace the xen_available() and
> kvm_available() functions from arch_init.c.
>
> Signed-off-by: Eduardo Habkost
> ---
> Cc: Richard Henderson
> Cc: Paolo Bonzini
> Cc: Claudio Fontana
> Cc: Roman Bolshakov
> ---
> include/sysemu/accel.h | 1 +
> accel/accel.c | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index e08b8ab8fa..a4a00c75c8 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -67,6 +67,7 @@ typedef struct AccelClass {
> OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
>
> AccelClass *accel_find(const char *opt_name);
> +bool accel_available(const char *name);
> int accel_init_machine(AccelState *accel, MachineState *ms);
>
> /* Called just before os_setup_post (ie just before drop OS privs) */
> diff --git a/accel/accel.c b/accel/accel.c
> index cb555e3b06..4a64a2b38a 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
> return ac;
> }
>
> +bool accel_available(const char *name)
> +{
> +return accel_find(name) != NULL;
accel_find() in its implementation allocates and then frees memory to generate
the string,
the user of accel_available() might be unaware and overuse leading to
fragmentation/performance issues?
> +}
> +
> int accel_init_machine(AccelState *accel, MachineState *ms)
> {
> AccelClass *acc = ACCEL_GET_CLASS(accel);
>
[PATCH v2 2/6] accel: accel_available() function
This function will be used to replace the xen_available() and
kvm_available() functions from arch_init.c.
Signed-off-by: Eduardo Habkost
---
Cc: Richard Henderson
Cc: Paolo Bonzini
Cc: Claudio Fontana
Cc: Roman Bolshakov
---
include/sysemu/accel.h | 1 +
accel/accel.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index e08b8ab8fa..a4a00c75c8 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -67,6 +67,7 @@ typedef struct AccelClass {
OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
AccelClass *accel_find(const char *opt_name);
+bool accel_available(const char *name);
int accel_init_machine(AccelState *accel, MachineState *ms);
/* Called just before os_setup_post (ie just before drop OS privs) */
diff --git a/accel/accel.c b/accel/accel.c
index cb555e3b06..4a64a2b38a 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
return ac;
}
+bool accel_available(const char *name)
+{
+return accel_find(name) != NULL;
+}
+
int accel_init_machine(AccelState *accel, MachineState *ms)
{
AccelClass *acc = ACCEL_GET_CLASS(accel);
--
2.28.0
