On 03/09/2015 06:30 PM, George Spelvin wrote: >> The tty_buffer is a lockless design because there is only ever a >> single producer (the uart driver) and a single consumer (the >> flush_to_ldisc() input worker). >> >> The uart driver produces filled (or partially filled) buffers at the >> tail and the input worker consumes buffers at the head. >> >> The lockless design requires one buffer in the list at all times, >> even if it's empty and consumed; the sentinel guarantees that condition >> when the tty is first created. > > I omitted the explanations because I'd figured it out; I'm actually quite > familiar with lockless designs. > > However, while the lockless design requires that the last buffer > never disappear, it's actually quite possible to *start* with a > NULL head pointer. But there is no value to it; on the contrary, it makes the code a mess to check for that initial condition everywhere. > An alternative would be to make the sentinel non-zero size and just > let it be recycled through the free list like everything else. Which would mean tty_buffer_init() could fail allocating the memory, which would mean tty_port_init() could fail... >> I suppose you're right about "tail->used" and "head->read" but please >> analyze each and every use if you plan to optimize those. > > I promise. The entire reason I'm asking all these questions is an > excess of caution. I stare at the code for a few days and *then* ask > "am I missing something?". > >>> Anyway, the steady state will typically be two 512-byte (size = 256) >>> buffers that are ping-ponged. So there should always be at least >>> 256 bytes of buffer space available without needing to allocate anything. > >> The flip buffer design was ditched years ago for good reason; what >> happens when the input worker has not even run yet on the previous input? > > I'm afraid I don't understand the point your rhetorical question is > trying to make. I understand that it's not a simple fixed-size buffer, > but in the usual case, when the ldisc is keeping up with the input, > you end up with two buffers acting a lot like flip buffers. On non-preemptive kernel with high line speed, you'll lose data. Same on preemptive kernel with a high load. IOW, the flip buffer construct requires the input worker to be run within a time deadline that the CFS does not guarantee. > Since the 8250 driver only ever requests space one byte at a time, > all buffers will be MIN_TTYB_SIZE, and minimum-size buffers are always > reallocated and not recycled. > > One buffer is allocated as soon as the first input byte arrives. Because > buffers are not pushed to the ldsic until the FIFO has been emptied, > it is very likely that a full FIFO will straddle the end of a buffer > and force the allocation of a second. > > Once that steady state is reached, a third buffer will not be allocated > unless the ldisc gets way behind, or a FIFO read has straddled both > buffers when an error occurs and forces the allocation of a !TTYB_NORMAL > buffer. > >>> I'll have to add some features to the tty_buffer interface to support a >>> no-allocation interface that reports if space is available and uses it >>> if it is. A bit invasive, but it seems like the only clean way. > >> You should be able to use the tty_buffer interface without modification >> from console write; the only other producer is the irq handler which is >> excluded by the console write with the port->lock. > > I know you're the expert here, but I really don't think that's right. > console_write may be called from somewhere deep in the kernel with > arbitrary locks held. So I can't safely call kmalloc(). Ahh, right. > And there's currently no way to make the call chain > tty_insert_flip_string_fixed_flag > -> __tty_buffer_request_room > -> tty_buffer_alloc > *not* call kmalloc(). > > Fundamentally, what I want to do is ask "how much space can you give > me with a guarantee of no kmalloc?" > > If there's no space, the fallback is don't empty the hardware FIFO and > risk overrun. That looks to be a difficult and thorny problem to overcome. >> Yep, I realize since I initially wrote about that the back in October, >> it's been lost in the sands of time (and you weren't in the conversation >> at the time), but I thought lock dropping should be removed. > > Whew! I spent a long time staring at the code thinking that I was missing > something significant. > >>> Another possible fix would be to test --max_count *before* >>> re-reading the LSR, so the errors would stay in the FIFO. > >> This would be my vote. > > It Will Be Done. > >>> Regarding the need for the unlock in the first place, I'm really >>> not seeing it. tty_flip_buffer_push() is just an alias for >>> tty_schedule_flip(), which just fills in port->buf.tail->commit and >>> calls schedule_work(port->buf.work). > >> The unlock is a remnant of the low_latency steering which was >> buggy and has been removed. I left it there because the -RT patches >> don't schedule the input worker but run it directly, and I didn't >> want to break that when there was no harm in leaving it. > > Ah! So there is a reason. I don't mind just adding a comment and > leaving it. > > But... looking at > https://www.kernel.org/pub/linux/kernel/projects/rt/3.18/ > I don't see the change you're referring to. > > - The call to tty_flip_buffer_push isn't changed > - tty_flip_buffer_push() isn't changed > - tty_schedule_flip() isn't changed > - schedule_work() isn't changed > - queue_work() isn't changed > - queue_work_on() is only changed very slightly (locking tweaks) > - __queue_work is only changed very slightly (locking tweaks) > > Does this still apply? The patch you're looking at applies to -stable-rt 3.14; that's where the scheduler/workqueue changes are. > By the way, the patches there to > > drivers/tty/serial/amba-pl011.c > drivers/tty/serial/omap-serial.c > > Look like they should be in mainline. I'm sure they'll make their way here if useful. >> Well, dropping the unlock allows normal tx to occur so that could >> break up the console output. > > It reorders the two, but it's a race condition anyway, so the ordering > isn't well-defined in the first place. > > Is there a more significant consequence I'm missing? Currently, complete lines are output by the console write. > Thank you very much for your thoughtful reply. No problem. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
