[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 37/54] plugin: expand the plugin_init function to include
From: |
Alex Bennée |
Subject: |
Re: [PATCH v6 37/54] plugin: expand the plugin_init function to include an info block |
Date: |
Thu, 24 Oct 2019 14:09:03 +0100 |
User-agent: |
mu4e 1.3.5; emacs 27.0.50 |
Aaron Lindsay OS <address@hidden> writes:
> On Oct 18 16:54, Alex Bennée wrote:
>>
>> Aaron Lindsay OS <address@hidden> writes:
>>
>> > On Oct 17 14:15, Alex Bennée wrote:
>> >> + const char *target_name;
>> >> + /* is this a full system emulation? */
>> >> + bool system_emulation;
>> >
>> > It seems that 'system_emulation' is meant primarily in opposition to
>> > user-mode. I'm wondering if this could/should this be an enum of the
>> > execution mode being used to allow for future expansion? Or, if your
>> > intention here is mostly to allow the user to detect when the *_vcpus
>> > variables are valid, could it be renamed or commented differently to
>> > make that link more clear?
>>
>> The only other operating mode that's ever been mooted is softmmu-user
>> (and no implementation has been done so far). Even then I don't think
>> that is a distinction that should be reported to the plugin as we are
>> trying not to leak implementation details.
>>
>> But yes the practical upshot is for system emulation you at least have
>> sort of bounded size for how many threads you may have running.
>
> Fair enough. My fear was that any other operating modes might require
> different plugin behavior, but it sounds like you think that unlikely.
> If we're attempting to keep the implementation details hidden, should we
> name this variable in terms of what it means for plugin implementations
> instead of what it means for QEMU? (Not sure this is a winner, but maybe
> something like "hardware_threading_model" )
>
>> >> + info->target_name = TARGET_NAME;
>> >> +#ifndef CONFIG_USER_ONLY
>> >> + MachineState *ms = MACHINE(qdev_get_machine());
>> >> + info->system_emulation = true;
>> >> + info->system.smp_vcpus = ms->smp.cpus;
>> >> + info->system.max_vcpus = ms->smp.max_cpus;
>> >> +#else
>> >> + info->system_emulation = false;
>> >
>> > Thinking "out loud" here - I wonder if it would be helpful to set the
>> > *_vcpus variables even for user mode here. It might allow unconditional
>> > allocation of "per-cpu" structures that the plugin might need - without
>> > first needing to check whether the *_vcpus variables were valid.
>>
>> but what too? It would certainly be wrong because any user-space process
>> could create (and destroy) thousands of threads.
>
> Hmm, right. To make sure I fully understand, does this mean that for
> user-mode, `vcpu_index` in the callback function pointer type below is
> actually the thread index?
No it is a monotonically increasing cpu_index for each new CPUState
created. So the first thread is 1 and the second is 2 no matter what the
thread id is.
> typedef void (*qemu_plugin_vcpu_simple_cb_t)(qemu_plugin_id_t id,
> unsigned int vcpu_index);
We don't actually use this prototype anymore. I had removed the concept
of vcpu_index from the translation time hooks (so people don't get any
ideas about it's significance there). However we do use vcpu_index with
the udata form.
> If so, do we have some max number of threads we support? I suppose we
> could set max_vcpux to that number, and smp_cpus to 1, though I'm not
> sure if that would be helpful or not.
>
>> We could consider just asking plugins to deal with threads with their
>> own __thread variables but in that case we'd need to expose some sort of
>> thread exit/cleanup method so they can collect data from threads and
>> safely place it somewhere else - but I suspect that is a hairy
>> programming model to impose on plugins.
>>
>> So far all the example plugins have just used locks to serialise things
>> and it's not been too bad. I guess we could do with an example that
>> tries to use this information to get an idea of how grungy the interface
>> is. Perhaps exposing the vCPUs at this point is pointless and we should
>> just stick to TARGET_NAME for now?
>
> I'm not sure. I liked the idea of exposing the vCPUs because it
> theoretically allows you to allocate per-cpu things up front, which can
> be helpful... but maybe forcing users to deal with dynamically
> allocating everything will make for more resilient plugins anyway?
So we do use it in the example plugins (hotpages tracks which vCPUs have
written to which pages). I think it is useful information for a plugin
but I think if you want per-vCPU structures in your plugin __thread is
going to be the easiest solution.
--
Alex Bennée
- [PATCH v6 13/54] plugin-gen: add module for TCG-related code, (continued)
- [PATCH v6 13/54] plugin-gen: add module for TCG-related code, Alex Bennée, 2019/10/17
- [PATCH v6 39/54] plugin: add qemu_plugin_outs helper, Alex Bennée, 2019/10/17
- [PATCH v6 29/54] target/alpha: fetch code with translator_ld, Alex Bennée, 2019/10/17
- [PATCH v6 50/54] tests/plugin: add hotpages to analyse memory access patterns, Alex Bennée, 2019/10/17
- [PATCH v6 47/54] tests/tcg: enable plugin testing, Alex Bennée, 2019/10/17
- [PATCH v6 37/54] plugin: expand the plugin_init function to include an info block, Alex Bennée, 2019/10/17
[PATCH v6 46/54] tests/tcg: drop test-i386-fprem from TESTS when not SLOW, Alex Bennée, 2019/10/17
[PATCH v6 48/54] tests/plugin: add a hotblocks plugin, Alex Bennée, 2019/10/17
[PATCH v6 34/54] translator: inject instrumentation from plugins, Alex Bennée, 2019/10/17
[PATCH v6 26/54] target/i386: fetch code with translator_ld, Alex Bennée, 2019/10/17
[PATCH v6 15/54] tcg: let plugins instrument virtual memory accesses, Alex Bennée, 2019/10/17
[PATCH v6 14/54] atomic_template: add inline trace/plugin helpers, Alex Bennée, 2019/10/17
[PATCH v6 18/54] *-user: notify plugin of exit, Alex Bennée, 2019/10/17
[PATCH v6 36/54] plugin: add API symbols to qemu-plugins.symbols, Alex Bennée, 2019/10/17
[PATCH v6 25/54] target/sh4: fetch code with translator_ld, Alex Bennée, 2019/10/17
[PATCH v6 33/54] target/openrisc: fetch code with translator_ld, Alex Bennée, 2019/10/17