Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
On Mon, Jul 13, 2015 at 03:44:04PM +0200, Vitaly Kuznetsov wrote: > Konrad Rzeszutek Wilk writes: > > > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote: > >> Log first 10 active grants of a domain. This function is going to be used > >> for soft reset, active grants on this path usually mean misbehaving > >> backends > >> refusing to release their mappings on shutdown. > > > > Is there an particular reason 10 was choosen instead of 42 for example :-) > > > > I was inpired by dump_pageframe_info() :-) Ah! > > > Also the 10 should probably have an #define for it. > > > > Ok, any preferred place/name for such define? I would say just include it at the start of the function (and #undef at the end), and maybe later on (if you want to) do a cleanup patch for this and dump_pageframe_info to be controlled by the same 'too long to display' logic. Which I would say could be a function that : - if 'loglvl=info' is used would do the 10. - if 'loglvl=all', then there is no limit. But that is such a minor thing. > > > Not sure I understand the usage case - except for development uses > > to report on the Xen console? But if that is the case why not > > use the 'g' on the ring console? > > If there is a misbehaving backend and this cases the domain to crash > right after the soft reset 'g' option will not be available and it won't > be clear what caused the domain to crash. Aah, that is good information. Please include that in the commit. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
Konrad Rzeszutek Wilk writes:
> On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
>> Log first 10 active grants of a domain. This function is going to be used
>> for soft reset, active grants on this path usually mean misbehaving backends
>> refusing to release their mappings on shutdown.
>
> Is there an particular reason 10 was choosen instead of 42 for example :-)
>
I was inpired by dump_pageframe_info() :-)
> Also the 10 should probably have an #define for it.
>
Ok, any preferred place/name for such define?
> Not sure I understand the usage case - except for development uses
> to report on the Xen console? But if that is the case why not
> use the 'g' on the ring console?
If there is a misbehaving backend and this cases the domain to crash
right after the soft reset 'g' option will not be available and it won't
be clear what caused the domain to crash.
>
>>
>> Signed-off-by: Vitaly Kuznetsov
>> ---
>> xen/common/grant_table.c | 31 +++
>> xen/include/xen/grant_table.h | 5 +
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index db5e5db..c67db28 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3309,6 +3309,37 @@ gnttab_release_mappings(
>> }
>> }
>>
>> +void grant_table_warn_active_grants(struct domain *d)
>> +{
>> +struct grant_table *gt = d->grant_table;
>> +struct active_grant_entry *act;
>> +grant_ref_t ref;
>> +unsigned int nr_active = 0;
>> +
>> +read_lock(>->lock);
>> +
>> +for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
>> +{
>> +act = active_entry_acquire(gt, ref);
>> +if ( !act->pin )
>> +{
>> +active_entry_release(act);
>> +continue;
>> +}
>> +
>> +nr_active++;
>> +if ( nr_active <= 10 )
>> +printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx"
>> + " (MFN: %lx)\n", d->domain_id, act->gfn, act->frame);
>> +active_entry_release(act);
>> +}
>> +
>> +if ( nr_active > 10 )
>> +printk(XENLOG_G_DEBUG "Dom%d has too many (%d) active grants"
>> + " to report\n", d->domain_id, nr_active);
>> +
>> +read_unlock(>->lock);
>> +}
>>
>> void
>> grant_table_destroy(
>> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
>> index 9c7b5a3..54005cc 100644
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -90,6 +90,11 @@ void grant_table_destroy(
>> struct domain *d);
>> void grant_table_init_vcpu(struct vcpu *v);
>>
>> +/*
>> + * Check if domain has active grants and log first 10 of them.
>> + */
>> +void grant_table_warn_active_grants(struct domain *d);
>> +
>> /* Domain death release of granted mappings of other domains' memory. */
>> void
>> gnttab_release_mappings(
>> --
>> 2.4.2
>>
--
Vitaly
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
"Jan Beulich" writes: On 13.07.15 at 11:08, wrote: >> On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote: >>> >>> On 10.07.15 at 18:24, wrote: >>> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote: >>> >> Log first 10 active grants of a domain. This function is going to be used >>> >> for soft reset, active grants on this path usually mean misbehaving >> backends >>> >> refusing to release their mappings on shutdown. >>> > >>> > Is there an particular reason 10 was choosen instead of 42 for example :-) >>> > >>> > Also the 10 should probably have an #define for it. >>> >>> Or even be command line controllable. >> >> That sounds like overkill to me, what's wrong with some random hardcoded >> number for a simple debug aid like this? > > From briefly looking at the code it seemed to be more than just a > debug aid (i.e. failing the operation if the count was exceeded). If > the number indeed only controls how many entries get printed, > then a #define certainly is fine. Yes, it is just a debug aid in cases something goes wrong in future. This info is supposed to be useful for hardware domain admin to help finding misbehaving backends. -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
>>> On 13.07.15 at 11:08, wrote: > On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote: >> >>> On 10.07.15 at 18:24, wrote: >> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote: >> >> Log first 10 active grants of a domain. This function is going to be used >> >> for soft reset, active grants on this path usually mean misbehaving > backends >> >> refusing to release their mappings on shutdown. >> > >> > Is there an particular reason 10 was choosen instead of 42 for example :-) >> > >> > Also the 10 should probably have an #define for it. >> >> Or even be command line controllable. > > That sounds like overkill to me, what's wrong with some random hardcoded > number for a simple debug aid like this? >From briefly looking at the code it seemed to be more than just a debug aid (i.e. failing the operation if the count was exceeded). If the number indeed only controls how many entries get printed, then a #define certainly is fine. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote: > >>> On 10.07.15 at 18:24, wrote: > > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote: > >> Log first 10 active grants of a domain. This function is going to be used > >> for soft reset, active grants on this path usually mean misbehaving > >> backends > >> refusing to release their mappings on shutdown. > > > > Is there an particular reason 10 was choosen instead of 42 for example :-) > > > > Also the 10 should probably have an #define for it. > > Or even be command line controllable. That sounds like overkill to me, what's wrong with some random hardcoded number for a simple debug aid like this? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
>>> On 10.07.15 at 18:24, wrote: > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote: >> Log first 10 active grants of a domain. This function is going to be used >> for soft reset, active grants on this path usually mean misbehaving backends >> refusing to release their mappings on shutdown. > > Is there an particular reason 10 was choosen instead of 42 for example :-) > > Also the 10 should probably have an #define for it. Or even be command line controllable. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()
On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
> Log first 10 active grants of a domain. This function is going to be used
> for soft reset, active grants on this path usually mean misbehaving backends
> refusing to release their mappings on shutdown.
Is there an particular reason 10 was choosen instead of 42 for example :-)
Also the 10 should probably have an #define for it.
Not sure I understand the usage case - except for development uses
to report on the Xen console? But if that is the case why not
use the 'g' on the ring console?
>
> Signed-off-by: Vitaly Kuznetsov
> ---
> xen/common/grant_table.c | 31 +++
> xen/include/xen/grant_table.h | 5 +
> 2 files changed, 36 insertions(+)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index db5e5db..c67db28 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3309,6 +3309,37 @@ gnttab_release_mappings(
> }
> }
>
> +void grant_table_warn_active_grants(struct domain *d)
> +{
> +struct grant_table *gt = d->grant_table;
> +struct active_grant_entry *act;
> +grant_ref_t ref;
> +unsigned int nr_active = 0;
> +
> +read_lock(>->lock);
> +
> +for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
> +{
> +act = active_entry_acquire(gt, ref);
> +if ( !act->pin )
> +{
> +active_entry_release(act);
> +continue;
> +}
> +
> +nr_active++;
> +if ( nr_active <= 10 )
> +printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx"
> + " (MFN: %lx)\n", d->domain_id, act->gfn, act->frame);
> +active_entry_release(act);
> +}
> +
> +if ( nr_active > 10 )
> +printk(XENLOG_G_DEBUG "Dom%d has too many (%d) active grants"
> + " to report\n", d->domain_id, nr_active);
> +
> +read_unlock(>->lock);
> +}
>
> void
> grant_table_destroy(
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 9c7b5a3..54005cc 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -90,6 +90,11 @@ void grant_table_destroy(
> struct domain *d);
> void grant_table_init_vcpu(struct vcpu *v);
>
> +/*
> + * Check if domain has active grants and log first 10 of them.
> + */
> +void grant_table_warn_active_grants(struct domain *d);
> +
> /* Domain death release of granted mappings of other domains' memory. */
> void
> gnttab_release_mappings(
> --
> 2.4.2
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
