| Message ID | 1435699387-32591-1-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/30/2015 02:23 PM, Timur Tabi wrote: > From: Shanker Donthineni <shankerd@codeaurora.org> > > Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle > reads/writes from/to DCC on secondary cores. Each core has its > own DCC device registers, so when a core reads or writes from/to DCC, > it only accesses its own DCC device. Since kernel code can run on > any core, every time the kernel wants to write to the console, it > might write to a different DCC. > > In SMP mode, Trace32 only uses the DCC on core 0. In AMP mode, it > creates multiple windows, and each window shows the DCC output > only from that core's DCC. The result is that console output is > either lost or scattered across windows. > > Selecting this option will enable code that serializes all console > input and output to core 0. The DCC driver will create input and > output FIFOs that all cores will use. Reads and writes from/to DCC > are handled by a workqueue that runs only core 0. > > Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> > Acked-by: Adam Wallis <awallis@codeaurora.org> > Signed-off-by: Timur Tabi <timur@codeaurora.org> > --- Maybe we should look into making the console number (i.e. ttyHVC0, ttyHVC1, etc.) correspond to the logical CPU number 0, 1, etc? We would need some hotplug notifier to tear down and restore the console when the CPU comes online and goes offline, but it may work out nicer than taking the approach this patch does.
Stephen Boyd wrote: > Maybe we should look into making the console number (i.e. ttyHVC0, > ttyHVC1, etc.) correspond to the logical CPU number 0, 1, etc? We would > need some hotplug notifier to tear down and restore the console when the > CPU comes online and goes offline, but it may work out nicer than taking > the approach this patch does. My understanding is that Trace32 only responds to core 0 in SMP mode. So if CPU0 goes offline, there's no point in migrating the thread to another CPU, because Trace32 won't listen for it anyway. Without this patch, console output is randomly scattered across CPUs because the put_chars call run on any CPU. Without consolidating all console output to one CPU, DCC is effectively useless. So I can make the changes you suggested, but I don't think that actually fixes anything. When CPU0 goes offline, what does schedule_work_on(0, actually do? If it does nothing, then the output FIFO will fill up, and put_chars will return 0, and that's it. Does CPU hotplug automatically take CPUs offline when the load is low? If so, then then thread could randomly bounce from CPU to CPU. Last I checked, ARM64 ACPI does not support discontiguous CPUs, so the boot CPU is always core "0".
On 06/30/2015 06:57 PM, Timur Tabi wrote: > Stephen Boyd wrote: >> Maybe we should look into making the console number (i.e. ttyHVC0, >> ttyHVC1, etc.) correspond to the logical CPU number 0, 1, etc? We would >> need some hotplug notifier to tear down and restore the console when the >> CPU comes online and goes offline, but it may work out nicer than taking >> the approach this patch does. > > My understanding is that Trace32 only responds to core 0 in SMP mode. > So if CPU0 goes offline, there's no point in migrating the thread to > another CPU, because Trace32 won't listen for it anyway. Without this > patch, console output is randomly scattered across CPUs because the > put_chars call run on any CPU. Without consolidating all console > output to one CPU, DCC is effectively useless. > > So I can make the changes you suggested, but I don't think that > actually fixes anything. It would at least fix the AMP case where you have one tty per CPU. If a CPU goes offline, the tty would be removed at the same time. The user could put the console on another CPU, or all of them, if they want to offline CPUs. It sounds like in the SMP case the tool is broken and it should do better about consolidating output from different CPUs into one place. If it really only listens to the boot CPU then making a tty per-CPU would work just the same as this patch even when the CPU goes offline. > When CPU0 goes offline, what does schedule_work_on(0, actually do? If > it does nothing, then the output FIFO will fill up, and put_chars will > return 0, and that's it. schedule_work_on() shouldn't be used when a CPU can go offline (see the comment above queue_work_on). I think it has to break affinity to run the work item on some other CPU. > > Does CPU hotplug automatically take CPUs offline when the load is low? > If so, then then thread could randomly bounce from CPU to CPU. Sorry, I should have been clearer. The thread would be bound to the CPU the tty corresponds to. No thread migration or bouncing. We would have to tear down and restore the tty on hotplug as well. I guess one problem here is that it doesn't do anything about the console. The console doesn't go through the khvcd thread like the tty does so we would still need some sort of dcc specific code to move the console writes to a particular CPU. And the downside there is that consoles are supposed to support atomic operations so that debugging information can always get out to the user. When we need to migrate the write to a particular CPU we might lose information if scheduling is broken or something. So can we fix the tool instead and keep writing out data on random CPUs?
On Jul 1, 2015, at 7:38 PM, Stephen Boyd wrote: > It would at least fix the AMP case where you have one tty per CPU. If a > CPU goes offline, the tty would be removed at the same time. The user > could put the console on another CPU, or all of them, if they want to > offline CPUs. How would the user put the TTY on another CPU? The command-line parameter is a boot-time configuration. I'm okay with adding that parameter, but we still have the problem of losing the TTY altogether if that CPU goes offline. I would like to see the TTY migrated to another CPU, but if I don't know if that makes sense either. Is it possible for CPUs to go randomly offline temporarily to manage load? To be clear, I'm okay with adding a hotplug hook that shuts down the thread if the TTY CPU goes offline, so that we don't attempt to schedule a thread on a CPU that's offline. I am concerned about whether hotplug can automatically offline the TTY CPU without warning, and suddenly we don't have TTY any more. Is there a way to prevent the TTY CPU from going offline via hotplug? A way of saying, "if you want to offline a CPU, don't offline this one?" > It sounds like in the SMP case the tool is broken and it should do > better about consolidating output from different CPUs into one place. If > it really only listens to the boot CPU then making a tty per-CPU would > work just the same as this patch even when the CPU goes offline. Yes, the tool is broken, but that's not the only problem. Each CPU has its own DCC, and since the console driver can run on any thread, it will scatter console output across all the CPUs. So any tool that listens on DCC is going to have this problem on an SMP system. I'm pretty sure Trace32 isn't the only one. >> When CPU0 goes offline, what does schedule_work_on(0, actually do? If >> it does nothing, then the output FIFO will fill up, and put_chars will >> return 0, and that's it. > > schedule_work_on() shouldn't be used when a CPU can go offline (see the > comment above queue_work_on). I think it has to break affinity to run > the work item on some other CPU. So if I run schedule_work_on() on a CPU, and that CPU is offline, it will just schedule the workqueue on some other random CPU? >> Does CPU hotplug automatically take CPUs offline when the load is low? >> If so, then then thread could randomly bounce from CPU to CPU. > > Sorry, I should have been clearer. The thread would be bound to the CPU > the tty corresponds to. No thread migration or bouncing. We would have > to tear down and restore the tty on hotplug as well. I guess one problem > here is that it doesn't do anything about the console. The console > doesn't go through the khvcd thread like the tty does so we would still > need some sort of dcc specific code to move the console writes to a > particular CPU. And the downside there is that consoles are supposed to > support atomic operations so that debugging information can always get > out to the user. When we need to migrate the write to a particular CPU > we might lose information if scheduling is broken or something. I'm not sure I understand all that. What do you mean by the thread is bound to the CPU that the TTY correspond to? When a kernel thread (running on some CPU) calls printk(), doesn't the call to the HVC driver occur also on that CPU? That's the whole reason behind this patch -- that each call to printk() results in a DCC write that occurs on a different CPU. > So can we fix the tool instead and keep writing out data on random CPUs? No, we can't. The impression I got that Lauterbach is not responsive to feature requests like this one. And again, this problem really occurs with any tool that listens to DCC on an SMP system.
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig index 8902f9b..2c6883c 100644 --- a/drivers/tty/hvc/Kconfig +++ b/drivers/tty/hvc/Kconfig @@ -95,6 +95,27 @@ config HVC_DCC driver. This console is used through a JTAG only on ARM. If you don't have a JTAG then you probably don't want this option. +config HVC_DCC_SERIALIZE_SMP + bool "Use DCC only on core 0" + depends on SMP && HVC_DCC + help + Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle + reads/writes from/to DCC on more than one core. Each core has its + own DCC device registers, so when a core reads or writes from/to DCC, + it only accesses its own DCC device. Since kernel code can run on + any core, every time the kernel wants to write to the console, it + might write to a different DCC. + + In SMP mode, Trace32 only uses the DCC on core 0. In AMP mode, it + creates multiple windows, and each window shows the DCC output + only from that core's DCC. The result is that console output is + either lost or scattered across windows. + + Selecting this option will enable code that serializes all console + input and output to core 0. The DCC driver will create input and + output FIFOs that all cores will use. Reads and writes from/to DCC + are handled by a workqueue that runs only core 0. + config HVC_BFIN_JTAG bool "Blackfin JTAG console" depends on BLACKFIN diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c index 809920d..33657dc 100644 --- a/drivers/tty/hvc/hvc_dcc.c +++ b/drivers/tty/hvc/hvc_dcc.c @@ -11,6 +11,10 @@ */ #include <linux/init.h> +#include <linux/kfifo.h> +#include <linux/spinlock.h> +#include <linux/moduleparam.h> +#include <linux/console.h> #include <asm/dcc.h> #include <asm/processor.h> @@ -48,26 +52,177 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count) return i; } +/* + * Check if the DCC is enabled. If CONFIG_HVC_DCC_SERIALIZE_SMP is enabled, + * then we assume then this function will be called first on core 0. That + * way, dcc_core0_available will be true only if it's available on core 0. + */ static bool hvc_dcc_check(void) { unsigned long time = jiffies + (HZ / 10); +#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP + static bool dcc_core0_available; + + /* + * If we're not on core 0, but we previously confirmed that DCC is + * active, then just return true. + */ + if (smp_processor_id() && dcc_core0_available) + return true; +#endif + /* Write a test character to check if it is handled */ __dcc_putchar('\n'); while (time_is_after_jiffies(time)) { - if (!(__dcc_getstatus() & DCC_STATUS_TX)) + if (!(__dcc_getstatus() & DCC_STATUS_TX)) { +#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP + dcc_core0_available = true; +#endif return true; + } } return false; } +#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP + +static void dcc_put_work_fn(struct work_struct *work); +static void dcc_get_work_fn(struct work_struct *work); +static DECLARE_WORK(dcc_pwork, dcc_put_work_fn); +static DECLARE_WORK(dcc_gwork, dcc_get_work_fn); +static DEFINE_SPINLOCK(dcc_lock); +static DEFINE_KFIFO(inbuf, unsigned char, 128); +static DEFINE_KFIFO(outbuf, unsigned char, 1024); + +/* + * Workqueue function that writes the output FIFO to the DCC on core 0. + */ +static void dcc_put_work_fn(struct work_struct *work) +{ + unsigned char ch; + + spin_lock(&dcc_lock); + + /* While there's data in the output FIFO, write it to the DCC */ + while (kfifo_get(&outbuf, &ch)) + hvc_dcc_put_chars(0, &ch, 1); + + /* While we're at it, check for any input characters */ + while (!kfifo_is_full(&inbuf)) { + if (!hvc_dcc_get_chars(0, &ch, 1)) + break; + kfifo_put(&inbuf, ch); + } + + spin_unlock(&dcc_lock); +} + +/* + * Workqueue function that reads characters from DCC and puts them into the + * input FIFO. + */ +static void dcc_get_work_fn(struct work_struct *work) +{ + unsigned char ch; + + /* + * Read characters from DCC and put them into the input FIFO, as + * long as there is room and we have characters to read. + */ + spin_lock(&dcc_lock); + + while (!kfifo_is_full(&inbuf)) { + if (!hvc_dcc_get_chars(0, &ch, 1)) + break; + kfifo_put(&inbuf, ch); + } + spin_unlock(&dcc_lock); +} + +/* + * Write characters directly to the DCC if we're on core 0 and the FIFO + * is empty, or write them to the FIFO if we're not. + */ +static int hvc_dcc0_put_chars(uint32_t vt, const char *buf, + int count) +{ + int len; + + spin_lock(&dcc_lock); + if (smp_processor_id() || (!kfifo_is_empty(&outbuf))) { + len = kfifo_in(&outbuf, buf, count); + spin_unlock(&dcc_lock); + /* + * We just push data to the output FIFO, so schedule the + * workqueue that will actually write that data to DCC. + */ + schedule_work_on(0, &dcc_pwork); + return len; + } + + /* + * If we're already on core 0, and the FIFO is empty, then just + * write the data to DCC. + */ + len = hvc_dcc_put_chars(vt, buf, count); + spin_unlock(&dcc_lock); + + return len; +} + +/* + * Read characters directly from the DCC if we're on core 0 and the FIFO + * is empty, or read them from the FIFO if we're not. + */ +static int hvc_dcc0_get_chars(uint32_t vt, char *buf, int count) +{ + int len; + + spin_lock(&dcc_lock); + + if (smp_processor_id() || (!kfifo_is_empty(&inbuf))) { + len = kfifo_out(&inbuf, buf, count); + spin_unlock(&dcc_lock); + + /* + * If the FIFO was empty, there may be characters in the DCC + * that we haven't read yet. Schedule a workqueue to fill + * the input FIFO, so that the next time this function is + * called, we'll have data. + */ + if (!len) + schedule_work_on(0, &dcc_gwork); + + return len; + } + + /* + * If we're already on core 0, and the FIFO is empty, then just + * read the data from DCC. + */ + len = hvc_dcc_get_chars(vt, buf, count); + spin_unlock(&dcc_lock); + + return len; +} + +static const struct hv_ops hvc_dcc_get_put_ops = { + .get_chars = hvc_dcc0_get_chars, + .put_chars = hvc_dcc0_put_chars, +}; + +#else + static const struct hv_ops hvc_dcc_get_put_ops = { .get_chars = hvc_dcc_get_chars, .put_chars = hvc_dcc_put_chars, }; +#endif + static int __init hvc_dcc_console_init(void) { if (!hvc_dcc_check())