Re: [Xen-devel] [PATCH 2/3] xen: RCU: make the period of the idle timer configurable.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.09.17 at 19:48, <dario.faggioli@xxxxxxxxxx> wrote:
> On Tue, 2017-09-26 at 09:14 -0600, Jan Beulich wrote:
>> > > > On 15.09.17 at 20:01, <dario.faggioli@xxxxxxxxxx> wrote:
>> > --- a/xen/common/rcupdate.c
>> > +++ b/xen/common/rcupdate.c
>> > +    int ret = 0;
>> > +
>> > +    if ( MILLISECS(period) > IDLE_TIMER_PERIOD_MAX )
>> > +    {
>> > +        printk("WARNING: rcu_idle_timer_period_ms must be <
>> > %"PRI_stime"\n",
>> > +               IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
>> > +        ret = -EINVAL;
>> > +    }
>> > +    else
>> > +        idle_timer_period = MILLISECS(period);
>> > +
>> > +    printk("RCU idle timer period: %lums\n", period);
>> > +
>> > +    return ret;
>> > +}
>> > +custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);
>> 
>> Does this really need handling as custom_param(). I.e. wouldn't
>> integer_param() plus sanitizing in rcu_init() suffice? 
>>
> I did it like that in the first place. But then I did not like the
> overall look of the patch, so I changed the approach.
> 
> I can put it back together the integer_param() variant, and you'll tell
> me which one you like better.
> 
>> That would
>> also make the log message be printed uniformly - merely echoing
>> the value from the command line doesn't look very useful to me.
>>
> Mmm.. Sorry, but I don't get this part.

In this version of the patch you log the message if and only if
the command line option was specified. This means that
- without command line option the log won't tell us anything
- with a valid command line option the log will tell us the same
  thing twice (once via the logged command line, and another
  time in the message you add)
- with an invalid command line option you'll be issuing both a
  warning and the message reporting the used value
I can live with the new message duplicating the command line
information (i.e. there's no point in suppressing the new log
message if the command line option was used), but (a) there
shouldn't be two messages (or really three, as returning
-EINVAL will trigger another message in the caller) and (b) if
knowing the period of the timer is something you consider
worth logging, it should be logged even when there's no
command line option.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel