>> 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. There's also code to handle the size==0 sentinel specially. I think you're right the NULL check would be messier, but it's not a huge mess. (Not that I necessarily intend to do this; it's just one alternative to explore and see what's cleanest.) >> 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... Er, that wasn't the idea. The sentinel would still be part of the tty_bufhead structure, and thus preallocated with it; it would just have some actual data[] appended. The only change to tty_buffer_init would be - tty_buffer_reset(&buf->sentinel, 0); + tty_buffer_reset(&buf->sentinel, MIN_TTYB_SIZE); >>>> 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. I'm still not getting it. It seems that you're describing the problems with the old flip buffer that led to it getting replaced. I understand the reasons for the change. What I don't understand is *why* you're describing old problems in code that doesn't exist any more. How do they affect tweaks to the current design? My point is that Prasad's use case requires, at most, receive buffer space equal to the size of serial8250_console_write output. And the current design allocates (and never frees) buffer space equal to the peak usage plus one buffer. (512 bytes with no errors; 256 bytes with error flags). So if I'm willing to settle for "probably", then when serial8250_console_write() is called, there's probably that much space available. I'm hoping that's enough preallocation for Pradad's use case. >> 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. By the way, I words two swapped. Minimum-size buffers *are* recycled (via the tty_bufhead's free list) and *not* de/reallocated. >> 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. It's not really that difficult, but it *does* require revision to the tty_buffer interface. Since the polling code only adds one byte at a time, I can do it with one function added to tty_buffer.h: static unsigned char * char_buf_ptr_noalloc(struct tty_bufhead *buf, char flag); This takes the proposed flag, and either returns a pointer to the space to put the character, or NULL. If it returns NULL, the rx_char() function will stash the flags in lsr_saved_flags and stop. If it returns non-null, the rx_char() function will read the UART_RX register, insert it at the returned pointer, and increment buf->head.used. (Alternatively, it could return a boolean and get the pointer from char_buf_ptr(buf->head, buf->head.used++).) By the way, there's also a performance bug in tty_insert_flip_char. If (tb->flags & TTYB_NORMAL), then tb->used is allowed to go up to 2*tb->size, as the __tty_buffer_request_room code makes clear. But tty_insert_flip_char() doesn't know that and will take the slow path for the second half of every buffer. >> - 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. Er, I checked again and I really don't think so. First of all, the patch at https://www.kernel.org/pub/linux/kernel/projects/rt/3.18/patch-3.18.9-rt4.patch.xz applies cleanly to v3.18.9. Second, looking at v3.14.33-rt30 from https://git.kernel.org/cgit/linux/kernel/git/rt/linux-stable-rt.git/log/?h=v3.14-rt The only changes to the queuing functions in kernel/workqueue.c and include/linux/workqueue.h are the ones I described: the addition of a new "pendingb_lock" and a lot of - local_irq_save(flags); + local_lock_irqsave(pendingb_lock, flags); - local_irq_restore(flags); + local_unlock_irqrestore(pendingb_lock, flags); Spedifically, I reviewed the output of git log -p v3.14.34..v3.14.34-rt31 -- kernel/workqueue.c include/linux/workqueue.h There are additional locking changes to the worker tasks that take work off the queues, but nothing that looks remotely like conversion of queued work to immediate calls. >> 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. Er, yes, and not dropping the lock in non-console write won't change that, so I don't understand how it's a change of any significance. Since console writes disable serial interrupts for the duration, they're always atomic. All that will happen is, if the console write happens to race with a call to serial8250_rx_chars, serial8250_handle_irq might output one more Tx FIFO worth of bytes before the console write. -- 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
