[PATCH] rtems_termios_puts: Copy and write more than one char at once

[Sebastian Huber]

On 08/07/2014 09:38 PM, Kolja Waschk wrote:
> ---
>   cpukit/libcsupport/src/termios.c | 74 +++++++++++++++++++++++++++-------------
>   1 file changed, 51 insertions(+), 23 deletions(-)
>
> diff --git a/cpukit/libcsupport/src/termios.c b/cpukit/libcsupport/src/termios.c
> index 2448ea1..d32ab74 100644
> --- a/cpukit/libcsupport/src/termios.c
> +++ b/cpukit/libcsupport/src/termios.c
> @@ -879,7 +879,7 @@ rtems_termios_ioctl (void *arg)
>         tty->rawInBufSemaphoreTimeout = RTEMS_NO_TIMEOUT;
>         tty->rawInBufSemaphoreFirstTimeout = RTEMS_NO_TIMEOUT;
>       } else {
> -      tty->vtimeTicks = tty->termios.c_cc[VTIME] *
> +      tty->vtimeTicks = tty->termios.c_cc[VTIME] *
>                       rtems_clock_get_ticks_per_second() / 10;
>         if (tty->termios.c_cc[VTIME]) {
>           tty->rawInBufSemaphoreOptions = RTEMS_WAIT;
> @@ -984,26 +984,19 @@ rtems_termios_puts (
>     unsigned int newHead;
>     rtems_interrupt_lock_context lock_context;
>     rtems_status_code sc;
> +  size_t partlen;
>   
>     if (tty->handler.mode == TERMIOS_POLLED) {
>       (*tty->handler.write)(tty, buf, len);
>       return;
>     }
> -  newHead = tty->rawOutBuf.Head;
> +
>     while (len) {
> -    /*
> -     * Performance improvement could be made here.
> -     * Copy multiple bytes to raw buffer:
> -     * if (len > 1) && (space to buffer end, or tail > 1)
> -     *  ncopy = MIN (len, space to buffer end or tail)
> -     *  memcpy (raw buffer, buf, ncopy)
> -     *  buf += ncopy
> -     *  len -= ncopy
> -     *
> -     * To minimize latency, the memcpy should be done
> -     * with interrupts enabled.
> -     */
> -    newHead = (newHead + 1) % tty->rawOutBuf.Size;
> +    /* Check space for at least one char */
> +    newHead = tty->rawOutBuf.Head + 1;
> +    if (newHead >= tty->rawOutBuf.Size)
> +      newHead -= tty->rawOutBuf.Size;
> +
>       rtems_termios_interrupt_lock_acquire (tty, &lock_context);
>       while (newHead == tty->rawOutBuf.Tail) {
>         tty->rawOutBufState = rob_wait;
> @@ -1014,21 +1007,56 @@ rtems_termios_puts (
>           rtems_fatal_error_occurred (sc);
>         rtems_termios_interrupt_lock_acquire (tty, &lock_context);
>       }
> -    tty->rawOutBuf.theBuf[tty->rawOutBuf.Head] = *buf++;
> +
> +    /* Copy as much chars as fit until current tail or end of ring buffer */
> +    partlen = len;
> +    if (tty->rawOutBuf.Tail > tty->rawOutBuf.Head) {
> +      /* Available space is contiguous from Head to Tail */
> +      size_t available = tty->rawOutBuf.Tail - tty->rawOutBuf.Head - 1;
> +      if (partlen > available)
> +        partlen = available;
> +    } else {
> +      /* Available space wraps at buffer end. To keep code simple, utilize
> +         only the part from Head to end during this iteration */
> +      size_t available = tty->rawOutBuf.Size - tty->rawOutBuf.Head;
> +      if (partlen > available)
> +        partlen = available;
> +    }

Maybe we should add helper functions for the buffer consecutive free and 
available chars, e.g.

static unsigned int
bufConsecutiveAvailable(
   unsigned int head,
   unsigned int tail,
   unsigned int size
)
{
   if (tail > head)
     return size - tail;
   else
     return head - tail;
}

static unsigned int
bufConsecutiveFree(
   unsigned int head,
   unsigned int tail,
   unsigned int size
)
{
   if (tail > head)
     return tail - head - 1;
   else
     return size - head;
}

since this is used on several places.

> +
> +    /* To minimize latency, the memcpy should be done
> +     * with interrupts enabled (TBD) */
> +    memcpy(&tty->rawOutBuf.theBuf[tty->rawOutBuf.Head], buf, partlen);
> +    newHead = tty->rawOutBuf.Head + partlen;
> +    if (newHead >= tty->rawOutBuf.Size)
> +      newHead -= tty->rawOutBuf.Size;
>       tty->rawOutBuf.Head = newHead;
> +    buf += partlen;
> +
>       if (tty->rawOutBufState == rob_idle) {
> -      /* check, whether XOFF has been received */
> -      if (!(tty->flow_ctrl & FL_ORCVXOF)) {
> -        (*tty->handler.write)(
> -          tty, &tty->rawOutBuf.theBuf[tty->rawOutBuf.Tail],1);

It should be possible to use the same logic as in 
rtems_termios_refill_transmitter() here or even introduce a common 
helper function for this block:

     /* check, whether output should stop due to received XOFF */
     else if ((tty->flow_ctrl & (FL_MDXON | FL_ORCVXOF))
        ==                (FL_MDXON | FL_ORCVXOF)) {
       /* Buffer not empty, but output stops due to XOFF */
       /* set flag, that output has been stopped */
       tty->flow_ctrl |= FL_OSTOP;
       tty->rawOutBufState = rob_busy; /*apm*/
       (*tty->handler.write) (tty, NULL, 0);
       nToSend = 0;
     } else {
       /*
        * Buffer not empty, start tranmitter
        */
       if (newTail > tty->rawOutBuf.Head)
         nToSend = tty->rawOutBuf.Size - newTail;
       else
         nToSend = tty->rawOutBuf.Head - newTail;
       /* when flow control XON or XOF, don't send blocks of data */
       /* to allow fast reaction on incoming flow ctrl and low latency*/
       /* for outgoing flow control */
       if (tty->flow_ctrl & (FL_MDXON | FL_MDXOF)) {
         nToSend = 1;
       }
       tty->rawOutBufState = rob_busy; /*apm*/
       (*tty->handler.write)(
         tty, &tty->rawOutBuf.theBuf[newTail], nToSend);
     }
     tty->rawOutBuf.Tail = newTail; /*apm*/
   }

   rtems_termios_interrupt_lock_release (tty, &lock_context);

> +      if (tty->flow_ctrl & FL_MDXOF) {
> +        /* Write only one byte at a time if using XON/XOFF flow ctrl */
> +        /* check, whether XOFF has been received */
> +        if (!(tty->flow_ctrl & FL_ORCVXOF)) {
> +          (*tty->handler.write)(
> +            tty, &tty->rawOutBuf.theBuf[tty->rawOutBuf.Tail], 1);
> +        } else {
> +          /* remember that output has been stopped due to flow ctrl*/
> +          tty->flow_ctrl |= FL_OSTOP;
> +        }
>         } else {
> -        /* remember that output has been stopped due to flow ctrl*/
> -        tty->flow_ctrl |= FL_OSTOP;
> +        size_t nwaiting;
> +        if (tty->rawOutBuf.Head > tty->rawOutBuf.Tail)
> +          nwaiting = tty->rawOutBuf.Head - tty->rawOutBuf.Tail;
> +        else
> +          nwaiting = tty->rawOutBuf.Size - tty->rawOutBuf.Tail;
> +
> +        (*tty->handler.write)(
> +          tty, &tty->rawOutBuf.theBuf[tty->rawOutBuf.Tail], nwaiting);
>         }
>         tty->rawOutBufState = rob_busy;
>       }
>       rtems_termios_interrupt_lock_release (tty, &lock_context);
> -    len--;
> +    len -= partlen;
>     }
>   }
>   


-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.