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
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 >
>>> 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
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 >
>>> 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
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
>>> 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
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 >
>>> 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
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