ns16550.c's poll_port variable

ns16550.c's poll_port variable

[Jan Beulich]

Hi Keir,

does this really need to be a per-CPU variable? Can't a timer run only
once at a time on the entire system (the more that it gets re-armed only
at the end of the poll function, whereas the variable is consumed at the
start), and hence the variable can be a simple one? Or else, what
subtlety am I overlooking?

Thanks, Jan

Re: ns16550.c's poll_port variable

[Keir Fraser]

On 04/05/2012 09:18, "Jan Beulich" <JBeulich@suse.com> wrote:

> Hi Keir,
> 
> does this really need to be a per-CPU variable? Can't a timer run only
> once at a time on the entire system (the more that it gets re-armed only
> at the end of the poll function, whereas the variable is consumed at the
> start), and hence the variable can be a simple one? Or else, what
> subtlety am I overlooking?

We set up a timer per initialised uart. There can be more than one (although
granted it is rare).

 -- Keir

> Thanks, Jan
> 

Re: ns16550.c's poll_port variable

[Jan Beulich]

>>> On 04.05.12 at 10:25, Keir Fraser <keir.xen@gmail.com> wrote:
> On 04/05/2012 09:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>> does this really need to be a per-CPU variable? Can't a timer run only
>> once at a time on the entire system (the more that it gets re-armed only
>> at the end of the poll function, whereas the variable is consumed at the
>> start), and hence the variable can be a simple one? Or else, what
>> subtlety am I overlooking?
> 
> We set up a timer per initialised uart. There can be more than one (although
> granted it is rare).

Is that actually useful? console.c can't really use more than one at a
time (for there being a single sercon_handle), so perhaps it would be
a good thing to avoid the setup for those that aren't actively used,
e.g. by not calling ->init_postirq() for other than the used one?

Oh, wait, with crash_debug there can be a second call to
serial_parse_handle(), so in that case serial console and gdb stub
may run through different ports. With crash_debug off by default,
wouldn't it make sense to optimize for that common case, so that
specifying both "com1=" and "com2=" on the command line wouldn't
needlessly consume resources (as serial_parse_handle() can easily
tag which ports it got called for)? Or would you rather take the
position of expecting people to remove unnecessary command line
options, or live with them having side effects?

Jan

Re: ns16550.c's poll_port variable

[Keir Fraser]

On 04/05/2012 10:12, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 04.05.12 at 10:25, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 04/05/2012 09:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> does this really need to be a per-CPU variable? Can't a timer run only
>>> once at a time on the entire system (the more that it gets re-armed only
>>> at the end of the poll function, whereas the variable is consumed at the
>>> start), and hence the variable can be a simple one? Or else, what
>>> subtlety am I overlooking?
>> 
>> We set up a timer per initialised uart. There can be more than one (although
>> granted it is rare).
> 
> Is that actually useful? console.c can't really use more than one at a
> time (for there being a single sercon_handle), so perhaps it would be
> a good thing to avoid the setup for those that aren't actively used,
> e.g. by not calling ->init_postirq() for other than the used one?
> 
> Oh, wait, with crash_debug there can be a second call to
> serial_parse_handle(), so in that case serial console and gdb stub
> may run through different ports. With crash_debug off by default,
> wouldn't it make sense to optimize for that common case, so that
> specifying both "com1=" and "com2=" on the command line wouldn't
> needlessly consume resources (as serial_parse_handle() can easily
> tag which ports it got called for)? Or would you rather take the
> position of expecting people to remove unnecessary command line
> options, or live with them having side effects?

I'm unclear on exactly what you want to optimise away? Certainly the 8 bytes
per CPU of the poll_port variable isn't worth much optimising effort.

 -- Keir

> Jan
> 

Re: ns16550.c's poll_port variable

[Jan Beulich]

>>> On 04.05.12 at 11:40, Keir Fraser <keir.xen@gmail.com> wrote:
> On 04/05/2012 10:12, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>>>> On 04.05.12 at 10:25, Keir Fraser <keir.xen@gmail.com> wrote:
>>> On 04/05/2012 09:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> does this really need to be a per-CPU variable? Can't a timer run only
>>>> once at a time on the entire system (the more that it gets re-armed only
>>>> at the end of the poll function, whereas the variable is consumed at the
>>>> start), and hence the variable can be a simple one? Or else, what
>>>> subtlety am I overlooking?
>>> 
>>> We set up a timer per initialised uart. There can be more than one (although
>>> granted it is rare).
>> 
>> Is that actually useful? console.c can't really use more than one at a
>> time (for there being a single sercon_handle), so perhaps it would be
>> a good thing to avoid the setup for those that aren't actively used,
>> e.g. by not calling ->init_postirq() for other than the used one?
>> 
>> Oh, wait, with crash_debug there can be a second call to
>> serial_parse_handle(), so in that case serial console and gdb stub
>> may run through different ports. With crash_debug off by default,
>> wouldn't it make sense to optimize for that common case, so that
>> specifying both "com1=" and "com2=" on the command line wouldn't
>> needlessly consume resources (as serial_parse_handle() can easily
>> tag which ports it got called for)? Or would you rather take the
>> position of expecting people to remove unnecessary command line
>> options, or live with them having side effects?
> 
> I'm unclear on exactly what you want to optimise away? Certainly the 8 bytes
> per CPU of the poll_port variable isn't worth much optimising effort.

Certainly not (and as you say they are actually needed, even if
only rarely). But the pointlessly running timer(s) might be, as might
the buffer(s) set up via serial_async_transmit().

Jan

Re: ns16550.c's poll_port variable

[Keir Fraser]

On 04/05/2012 12:05, "Jan Beulich" <JBeulich@suse.com> wrote:

>> I'm unclear on exactly what you want to optimise away? Certainly the 8 bytes
>> per CPU of the poll_port variable isn't worth much optimising effort.
> 
> Certainly not (and as you say they are actually needed, even if
> only rarely). But the pointlessly running timer(s) might be, as might
> the buffer(s) set up via serial_async_transmit().

We could delay {init,setup}_postirq until a corresponding serial handle has
been created via serial_parse_handle()? The logic might be a bit ugly and
spread across both serial.c and ns16550.c but not actually particularly
complicated?

 -- Keir

Re: ns16550.c's poll_port variable

[Jan Beulich]

>>> On 04.05.12 at 14:03, Keir Fraser <keir@xen.org> wrote:
> On 04/05/2012 12:05, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>> I'm unclear on exactly what you want to optimise away? Certainly the 8 bytes
>>> per CPU of the poll_port variable isn't worth much optimising effort.
>> 
>> Certainly not (and as you say they are actually needed, even if
>> only rarely). But the pointlessly running timer(s) might be, as might
>> the buffer(s) set up via serial_async_transmit().
> 
> We could delay {init,setup}_postirq until a corresponding serial handle has
> been created via serial_parse_handle()? The logic might be a bit ugly and
> spread across both serial.c and ns16550.c but not actually particularly
> complicated?

I think this can be done entirely in serial.c - serial_init_postirq()
would directly call any drivers that already got a handle parsed
for them, and serial_parse_handle() would need to call
->init_postirq() for any driver that didn't have it called already.
serial_suspend() and serial_resume() should then call drivers only
if they previously had ->init_postirq() called.

I definitely want to avoid putting any part of this into ns16550.c,
as it would need to be replicated for ARM's pl011.c as well as any
future ones (I'm now mostly done with an EHCI debug port driver,
but due to feature freeze won't be able to post this as other than
an RFC any time soon; xHCI appears to also have a debug port,
so in the future that might become a third alternative).

Jan

Re: ns16550.c's poll_port variable

[Keir Fraser]

On 04/05/2012 13:46, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 04.05.12 at 14:03, Keir Fraser <keir@xen.org> wrote:
>> On 04/05/2012 12:05, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>>> I'm unclear on exactly what you want to optimise away? Certainly the 8
>>>> bytes
>>>> per CPU of the poll_port variable isn't worth much optimising effort.
>>> 
>>> Certainly not (and as you say they are actually needed, even if
>>> only rarely). But the pointlessly running timer(s) might be, as might
>>> the buffer(s) set up via serial_async_transmit().
>> 
>> We could delay {init,setup}_postirq until a corresponding serial handle has
>> been created via serial_parse_handle()? The logic might be a bit ugly and
>> spread across both serial.c and ns16550.c but not actually particularly
>> complicated?
> 
> I think this can be done entirely in serial.c - serial_init_postirq()
> would directly call any drivers that already got a handle parsed
> for them, and serial_parse_handle() would need to call
> ->init_postirq() for any driver that didn't have it called already.
> serial_suspend() and serial_resume() should then call drivers only
> if they previously had ->init_postirq() called.

Ah yes, that would work. Feel free to make a patch.

 -- Keir

> I definitely want to avoid putting any part of this into ns16550.c,
> as it would need to be replicated for ARM's pl011.c as well as any
> future ones (I'm now mostly done with an EHCI debug port driver,
> but due to feature freeze won't be able to post this as other than
> an RFC any time soon; xHCI appears to also have a debug port,
> so in the future that might become a third alternative).
> 
> Jan
> 

Re: ns16550.c's poll_port variable

[Jan Beulich]

>>> On 04.05.12 at 15:27, Keir Fraser <keir.xen@gmail.com> wrote:
> On 04/05/2012 13:46, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>>>> On 04.05.12 at 14:03, Keir Fraser <keir@xen.org> wrote:
>>> On 04/05/2012 12:05, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> 
>>>>> I'm unclear on exactly what you want to optimise away? Certainly the 8
>>>>> bytes
>>>>> per CPU of the poll_port variable isn't worth much optimising effort.
>>>> 
>>>> Certainly not (and as you say they are actually needed, even if
>>>> only rarely). But the pointlessly running timer(s) might be, as might
>>>> the buffer(s) set up via serial_async_transmit().
>>> 
>>> We could delay {init,setup}_postirq until a corresponding serial handle has
>>> been created via serial_parse_handle()? The logic might be a bit ugly and
>>> spread across both serial.c and ns16550.c but not actually particularly
>>> complicated?
>> 
>> I think this can be done entirely in serial.c - serial_init_postirq()
>> would directly call any drivers that already got a handle parsed
>> for them, and serial_parse_handle() would need to call
>> ->init_postirq() for any driver that didn't have it called already.
>> serial_suspend() and serial_resume() should then call drivers only
>> if they previously had ->init_postirq() called.
> 
> Ah yes, that would work. Feel free to make a patch.

That'll be on top of the other post-4.2 changes I'm in the process
of piling up, as this isn't really a bug fix.

Jan

Re: ns16550.c's poll_port variable

[Keir Fraser]

On 04/05/2012 14:55, "Jan Beulich" <JBeulich@suse.com> wrote:

>>> I think this can be done entirely in serial.c - serial_init_postirq()
>>> would directly call any drivers that already got a handle parsed
>>> for them, and serial_parse_handle() would need to call
>>> ->init_postirq() for any driver that didn't have it called already.
>>> serial_suspend() and serial_resume() should then call drivers only
>>> if they previously had ->init_postirq() called.
>> 
>> Ah yes, that would work. Feel free to make a patch.
> 
> That'll be on top of the other post-4.2 changes I'm in the process
> of piling up, as this isn't really a bug fix.

Agreed.

 -- Keir