| Message ID | 20201125205636.3305257-3-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | arch_init.c cleanup | expand |
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 <ehabkost@redhat.com> > --- > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Claudio Fontana <cfontana@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > --- > 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); >
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 <ehabkost@redhat.com> > > --- > > Cc: Richard Henderson <richard.henderson@linaro.org> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Claudio Fontana <cfontana@suse.de> > > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > > --- > > 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?
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 <ehabkost@redhat.com> >>> --- >>> Cc: Richard Henderson <richard.henderson@linaro.org> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Claudio Fontana <cfontana@suse.de> >>> Cc: Roman Bolshakov <r.bolshakov@yadro.com> >>> --- >>> 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
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
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
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.
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
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
On Wed, 25 Nov 2020 15:56:32 -0500 Eduardo Habkost <ehabkost@redhat.com> wrote: > This function will be used to replace the xen_available() and > kvm_available() functions from arch_init.c. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Claudio Fontana <cfontana@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > --- > include/sysemu/accel.h | 1 + > accel/accel.c | 5 +++++ > 2 files changed, 6 insertions(+) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Claudio Fontana <cfontana@suse.de> 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.
On 11/27/20 3:45 PM, Markus Armbruster wrote: > Claudio Fontana <cfontana@suse.de> 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
Claudio Fontana <cfontana@suse.de> writes: > On 11/27/20 3:45 PM, Markus Armbruster wrote: >> Claudio Fontana <cfontana@suse.de> 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.
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);
This function will be used to replace the xen_available() and kvm_available() functions from arch_init.c. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Claudio Fontana <cfontana@suse.de> Cc: Roman Bolshakov <r.bolshakov@yadro.com> --- include/sysemu/accel.h | 1 + accel/accel.c | 5 +++++ 2 files changed, 6 insertions(+)