Re: [PATCH v3] xen/console: Skip switching serial input to non existing domains

[Michal Orzel <michal.orzel@xxxxxxx>]

On 20/03/2023 12:17, Jan Beulich wrote:
> 
> 
> On 20.03.2023 09:19, Michal Orzel wrote:
>> @@ -483,15 +485,34 @@ struct domain *console_input_domain(void)
>>
>>  static void switch_serial_input(void)
>>  {
>> -    if ( console_rx == max_init_domid + 1 )
>> -    {
>> -        console_rx = 0;
>> -        printk("*** Serial input to Xen");
>> -    }
>> -    else
>> +    unsigned int next_rx = console_rx + 1;
>> +
>> +    /*
>> +     * Rotate among Xen, dom0 and boot-time created domUs while skipping
>> +     * switching serial input to non existing domains.
>> +     */
>> +    while ( next_rx <= max_console_rx + 1 )
>>      {
>> -        console_rx++;
>> -        printk("*** Serial input to DOM%d", console_rx - 1);
>> +        if ( next_rx == max_console_rx + 1 )
> 
> Part of the earlier problems stemmed from the comparison being == here.
> Could I talk you into using >= instead?
With the loop condition unmodified it would not make sense as it would be 
impossible.
However, because of what you wrote below, I will do this together with other 
modifications.

> 
>> +        {
>> +            console_rx = 0;
>> +            printk("*** Serial input to Xen");
>> +            break;
>> +        }
>> +        else
> 
> No need for "else" after "break" (or alike). Omitting it will not only
> decrease indentation, but also make more visible that the earlier if()
> won't "fall through".
> 
ok.

>> +        {
>> +            struct domain *d = rcu_lock_domain_by_id(next_rx - 1);
>> +
>> +            if ( d )
>> +            {
>> +                rcu_unlock_domain(d);
>> +                console_rx = next_rx;
>> +                printk("*** Serial input to DOM%d", console_rx - 1);
> 
> While I expect the compiler will transform this to using "next_rx"
> here anyway, I think it would be nice if it was written like this
> right away.
ok.

> 
> Since you touch the printk() anyway, please also switch to using the
> more applicable %u.
ok.

> 
> With the adjustments
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> One other transformation for you to consider is to switch to a base
> layout like
> 
>     unsigned int next_rx = console_rx;
>     while ( next_rx++ <= max_console_rx )
>     {
>         ...
>     }
> 
> i.e. without a separate increment at the bottom of the loop. Which,
> now that I've spelled it out, raises the question of why the outer
> loop needs a condition in the first place (because as written above
> it clearly is always true). So perhaps better (and more directly
> showing what's going on)
> 
>     unsigned int next_rx = console_rx;
>     for ( ; ; )
>     {
>         if ( next_rx++ >= max_console_rx )
>         ...
>     }
Makes sense to me so I will do this assuming that you agree on adding your Rb 
tag also
for this approach.

~Michal