| Message ID | 20170320100302.8656-1-aleksey.makarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue 2017-03-28 11:04:04, Sergey Senozhatsky wrote: > On (03/27/17 19:28), Aleksey Makarov wrote: > [..] > > > > + /* > > > > + * Maintain an invariant that will help to find if > > > > + * the matching console is preferred, see > > > > + * register_console(): > > > > + * > > > > + * The last non-braille console is always > > > > + * the preferred one. > > > > + */ > > > > + for (last = MAX_CMDLINECONSOLES - 1; > > > > + last >= 0 && !console_cmdline[last].name[0]; > > > > + last--) > > > > + ; > > > > > > This is a rather non-trivial code to find the last element. > > > I might make sense to count it in a global variable. > > > Then we might remove the check for console_cmdline[i].name[0] > > > also in the other for cycles and make them better readable. > > > > Having an additional variable console_cmdline_last pointing to the last element > > would require maintaining consistency between this variable and > > contents of console_cmdline. For the code we have it is not hard, but when code > > is changed we need to check this. Also there exists preferred_console that > > has almost the same meaning but it points not to the last element, but to the > > last non-braille element. Also we need to have a special value (-1) for this > > variable for empty array. So I personally would instead try to rewrite this: > > > > for (last = MAX_CMDLINECONSOLES - 1; last >= 0; last--) > > if (console_cmdline[last].name[0]) > > break; > > > > Is it better? If not, I will send a version with console_cmdline_last. > > personally I'm fine with the nested loop. the latest version > "for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..." > > is even easier to read. The number of elements is bumped on a single location, so there is not much to synchronize. The old approach was fine because the for cycles were needed anyway, they started on the 0th element, and NULL ended arrays are rather common practice. But we are searching the array from the end now. Also we use the for cycle just to get the number here. This is not a common practice and it makes the code more complicated and strange from my point of view. If you do not like -1, you could use console_cmdline_cnt and start with 0. I would actually do so because it is a common approach and easy to understand. > > so we do not just iterate console_cmdline anymore, but also modify it. > this, probably, has impact on the following scenario > > CPU0 CPU1 > > add_preferred_console() add_preferred_console() > __add_preferred_console() __add_preferred_console() > swap(i1, last) swap(i2, last) > > temp1 = i1 > i1 = last temp2 = i2 > last = temp1 i2 = last > last = temp2 > > so both i1 and i2 will point to 'last' now, IOW, we will have two > identical entries in console_cmdline, while i1 or i2 will be lost. > > > neither add_preferred_console() nor __add_preferred_console() have any > serialization. and I assume that we can call add_preferred_console() > concurrently, can't we? Very good point! Well, if this race exists, it was racy even before. Of course, the old race was only when new entry was added. It would be more visible now because also shuffling would be racy. OK, most add_preferred_console() calls are in functions that are defined as console_initcall(). They seem to be defined only when the respective drivers are built in. It seems that these initcalls are serialized, see console_init(). add_preferred_console() is used also in uart_add_one_port(): -> uart_add_one_port() -> of_console_check() -> add_preferred_console() But there the calls are synchronized as well via port_mutex. Finally, __add_preferred_console() is called also from console_setup(). It is called via do_early_param() even before the console_initcall() functions. It is serialized as well. If I did not miss anything, it would seem that __add_preferred_console() are called synchronously and only during boot by design. Best Regards, Petr
On (03/28/17 14:56), Petr Mladek wrote: [..] > > > Is it better? If not, I will send a version with console_cmdline_last. > > > > personally I'm fine with the nested loop. the latest version > > "for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..." > > > > is even easier to read. > > The number of elements is bumped on a single location, so there > is not much to synchronize. The old approach was fine because > the for cycles were needed anyway, they started on the 0th element, > and NULL ended arrays are rather common practice. > > But we are searching the array from the end now. Also we use the > for cycle just to get the number here. This is not a common > practice and it makes the code more complicated and strange from > my point of view. I'm fine with either way :) [..] > > neither add_preferred_console() nor __add_preferred_console() have any > > serialization. and I assume that we can call add_preferred_console() > > concurrently, can't we? [..] > If I did not miss anything, it would seem that > __add_preferred_console() are called synchronously > and only during boot by design. thanks. I think you are right. it's console_initcall or __init. -ss
On Thu 2017-03-30 14:55:46, Sergey Senozhatsky wrote: > On (03/28/17 14:56), Petr Mladek wrote: > [..] > > > > Is it better? If not, I will send a version with console_cmdline_last. > > > > > > personally I'm fine with the nested loop. the latest version > > > "for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..." > > > > > > is even easier to read. > > > > The number of elements is bumped on a single location, so there > > is not much to synchronize. The old approach was fine because > > the for cycles were needed anyway, they started on the 0th element, > > and NULL ended arrays are rather common practice. > > > > But we are searching the array from the end now. Also we use the > > for cycle just to get the number here. This is not a common > > practice and it makes the code more complicated and strange from > > my point of view. > > I'm fine with either way :) Alekesey, any chance to use the global variable to count used or point to the last element? I know that you have already spent a lot of time with it. It was great work. But the current solution of the cycle looks weird to me. Best Regards, Petr
On 04/04/2017 02:12 PM, Petr Mladek wrote: > On Thu 2017-03-30 14:55:46, Sergey Senozhatsky wrote: >> On (03/28/17 14:56), Petr Mladek wrote: [..] > Alekesey, any chance to use the global variable to count used or point > to the last element? > > I know that you have already spent a lot of time with it. It was great > work. But the current solution of the cycle looks weird to me. Sorry for the delay. I will send next version soon. Thank you Aleksey Makarov
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index fd752f0c8ef1..462036e7a767 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1909,8 +1909,28 @@ static int __add_preferred_console(char *name, int idx, char *options, i < MAX_CMDLINECONSOLES && c->name[0]; i++, c++) { if (strcmp(c->name, name) == 0 && c->index == idx) { - if (!brl_options) - preferred_console = i; + int last; + + if (brl_options) + return 0; + + /* + * Maintain an invariant that will help to find if + * the matching console is preferred, see + * register_console(): + * + * The last non-braille console is always + * the preferred one. + */ + for (last = MAX_CMDLINECONSOLES - 1; + last >= 0 && !console_cmdline[last].name[0]; + last--) + ; + + if (i != last) + swap(console_cmdline[i], console_cmdline[last]); + + preferred_console = last; return 0; } } @@ -2457,12 +2477,27 @@ void register_console(struct console *newcon) } /* - * See if this console matches one we selected on - * the command line. + * See if this console matches one we selected on the command line. + * + * There may be several entries in the console_cmdline array matching + * with the same console, one with newcon->match(), another by + * name/index: + * + * pl011,mmio,0x87e024000000,115200 -- added from SPCR + * ttyAMA0 -- added from command line + * + * Traverse the console_cmdline array in reverse order to be + * sure that if this console is preferred then it will be the first + * matching entry. We use the invariant that is maintained in + * __add_preferred_console(). */ - for (i = 0, c = console_cmdline; - i < MAX_CMDLINECONSOLES && c->name[0]; - i++, c++) { + for (i = MAX_CMDLINECONSOLES - 1; i >= 0; i--) { + + if (!console_cmdline[i].name[0]) + continue; + + c = console_cmdline + i; + if (!newcon->match || newcon->match(newcon, c->name, c->index, c->options) != 0) { /* default matching */
If a console was specified by ACPI SPCR table _and_ command line parameters like "console=ttyAMA0" _and_ "earlycon" were specified, then log messages appear twice. The root cause is that the code traverses the list of specified consoles (the `console_cmdline` array) and stops at the first match. But it may happen that the same console is referred by the elements of this array twice: pl011,mmio,0x87e024000000,115200 -- from SPCR ttyAMA0 -- from command line but in this case `preferred_console` points to the second entry and the flag CON_CONSDEV is not set, so bootconsole is not deregistered. To fix that, introduce an invariant "The last non-braille console is always the preferred one" on the entries of the console_cmdline array. Then traverse it in reverse order to be sure that if the console is preferred then it will be the first matching entry. Reported-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> --- v7 -> v8: - add an explanation to the comment how console_cmdline can contain entries referring to the same console - move the body of the function introduced in the previous version to cycle - don't panic() (Sergey Senozhatsky). Don't check this condition because the loop condition guarantees that it holds. - use swap() (Sergey Senozhatsky) v6 -> v7: - return back to v5 - leave the check for already registered entries and add a function that maintains the invariant. v5 -> v6: - drop v5 and continue to work on v4: - introduce _braille_is_braille_console(). It helps to split original loop into three parts: 1) search for braille console, 2) check for preferred_console, 3) match other entries so that these three parts do not intersect. - introduce for_each_console_cmdline() macros to traverse console_cmdline (Petr Mladek) kernel/printk/printk.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) -- 2.12.0