On Sunday 23 August 2009, George Spelvin wrote:
> Subject: usb/gadget/u_serial: Simplify output buffer implementation.
>
> There's no need to store buf_size in a variable, given that it's a
> compile-time cosntant. And since it's a pwoer of 2, use a different
> indexing scheme that lets us fill the circular buffer rather than
> needing to reserve one byte to disambiguate empty and full.
> ---
> > Patches accepted. :)
>
> Something like this?
This makes more than one patch. I count:
- using buf size as a compile time constant
- changing the index scheme
- a few gratuitous changes like
unsigned len;
...
len = ...;
becoming
unsigned len = ...;
- maybe more
One for just the first is a no-brainer. The second needs
review; by itself please. The third I'd rather not see.
> One question I have is whether it's worth removing
> the locking from gs_write_room and gs_chars_in_buffer. I avoided it for
> now, on the assumption that those aren't hot code paths, but it could
> be done.
No worth it for now.
> Does anyone have opinions on the matter?
>
> (Indeed, you could have independent read and write locking, so readers
> and writers could overlap, but that's a bigger design question that
> I don't feel like researching right now.)
>
> diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
> index fc6e709..8f0f1f8 100644
> --- a/drivers/usb/gadget/u_serial.c
> +++ b/drivers/usb/gadget/u_serial.c
> @@ -80,10 +80,8 @@
>
> /* circular buffer */
> struct gs_buf {
> - unsigned buf_size;
> - char *buf_buf;
> - char *buf_get;
> - char *buf_put;
> + char *buf_buf; /* WRITE_BUF_SIZE */
> + unsigned buf_get, buf_put; /* Offsets */
> };
>
> /*
> @@ -144,15 +142,13 @@ static unsigned n_ports;
> *
> * Allocate a circular buffer and all associated memory.
> */
> -static int gs_buf_alloc(struct gs_buf *gb, unsigned size)
> +static int gs_buf_alloc(struct gs_buf *gb)
> {
> - gb->buf_buf = kmalloc(size, GFP_KERNEL);
> + gb->buf_buf = kmalloc(WRITE_BUF_SIZE, GFP_KERNEL);
> if (gb->buf_buf == NULL)
> return -ENOMEM;
>
> - gb->buf_size = size;
> - gb->buf_put = gb->buf_buf;
> - gb->buf_get = gb->buf_buf;
> + gb->buf_put = gb->buf_get = 0;
>
> return 0;
> }
> @@ -161,6 +157,8 @@ static int gs_buf_alloc(struct gs_buf *gb, unsigned size)
> * gs_buf_free
> *
> * Free the buffer and all associated memory.
> + * Must be idempotent; we call this early if possible, and again on
> + * final disconnect.
> */
> static void gs_buf_free(struct gs_buf *gb)
> {
> @@ -185,9 +183,9 @@ static void gs_buf_clear(struct gs_buf *gb)
> * Return the number of bytes of data written into the circular
> * buffer.
> */
> -static unsigned gs_buf_data_avail(struct gs_buf *gb)
> +static unsigned gs_buf_data_avail(struct gs_buf const *gb)
> {
> - return (gb->buf_size + gb->buf_put - gb->buf_get) % gb->buf_size;
> + return gb->buf_put - gb->buf_get;
> }
>
> /*
> @@ -196,9 +194,9 @@ static unsigned gs_buf_data_avail(struct gs_buf *gb)
> * Return the number of bytes of space available in the circular
> * buffer.
> */
> -static unsigned gs_buf_space_avail(struct gs_buf *gb)
> +static unsigned gs_buf_space_avail(struct gs_buf const *gb)
> {
> - return (gb->buf_size + gb->buf_get - gb->buf_put - 1) % gb->buf_size;
> + return WRITE_BUF_SIZE - gs_buf_data_avail(gb);
> }
>
> /*
> @@ -212,27 +210,26 @@ static unsigned gs_buf_space_avail(struct gs_buf *gb)
> static unsigned
> gs_buf_put(struct gs_buf *gb, const char *buf, unsigned count)
> {
> - unsigned len;
> + unsigned len = gs_buf_space_avail(gb);
> + unsigned put_pos = gb->buf_put % WRITE_BUF_SIZE;
>
> - len = gs_buf_space_avail(gb);
> if (count > len)
> count = len;
>
> - if (count == 0)
> + if (count == 0) /* This short-cut isn't actually necessary */
> return 0;
>
> - len = gb->buf_buf + gb->buf_size - gb->buf_put;
> + len = WRITE_BUF_SIZE - put_pos;
> +
> if (count > len) {
> - memcpy(gb->buf_put, buf, len);
> - memcpy(gb->buf_buf, buf+len, count - len);
> - gb->buf_put = gb->buf_buf + count - len;
> + memcpy(gb->buf_buf + put_pos, buf, len);
> + len = count - len;
> + put_pos = 0;
> } else {
> - memcpy(gb->buf_put, buf, count);
> - if (count < len)
> - gb->buf_put += count;
> - else /* count == len */
> - gb->buf_put = gb->buf_buf;
> + len = count;
> }
> + memcpy(gb->buf_buf + put_pos, buf, len);
> + gb->buf_put += count;
>
> return count;
> }
> @@ -248,27 +245,25 @@ gs_buf_put(struct gs_buf *gb, const char *buf, unsigned count)
> static unsigned
> gs_buf_get(struct gs_buf *gb, char *buf, unsigned count)
> {
> - unsigned len;
> + unsigned len = gs_buf_data_avail(gb);
> + unsigned get_pos = gb->buf_get % WRITE_BUF_SIZE;
>
> - len = gs_buf_data_avail(gb);
> if (count > len)
> count = len;
>
> - if (count == 0)
> + if (count == 0) /* This short-cut isn't actually necessary */
> return 0;
>
> - len = gb->buf_buf + gb->buf_size - gb->buf_get;
> + len = WRITE_BUF_SIZE - get_pos;
> if (count > len) {
> - memcpy(buf, gb->buf_get, len);
> - memcpy(buf+len, gb->buf_buf, count - len);
> - gb->buf_get = gb->buf_buf + count - len;
> + memcpy(buf, gb->buf_buf + get_pos, len);
> + len = count - len;
> + get_pos = 0;
> } else {
> - memcpy(buf, gb->buf_get, count);
> - if (count < len)
> - gb->buf_get += count;
> - else /* count == len */
> - gb->buf_get = gb->buf_buf;
> + len = count;
> }
> + memcpy(buf, gb->buf_buf + get_pos, len);
> + gb->buf_get += count;
>
> return count;
> }
> @@ -325,14 +320,7 @@ void gs_free_req(struct usb_ep *ep, struct usb_request *req)
> static unsigned
> gs_send_packet(struct gs_port *port, char *packet, unsigned size)
> {
> - unsigned len;
> -
> - len = gs_buf_data_avail(&port->port_write_buf);
> - if (len < size)
> - size = len;
> - if (size != 0)
> - size = gs_buf_get(&port->port_write_buf, packet, size);
> - return size;
> + return gs_buf_get(&port->port_write_buf, packet, size);
> }
>
> /*
> @@ -760,7 +748,7 @@ static int gs_open(struct tty_struct *tty, struct file *file)
> if (port->port_write_buf.buf_buf == NULL) {
>
> spin_unlock_irq(&port->port_lock);
> - status = gs_buf_alloc(&port->port_write_buf, WRITE_BUF_SIZE);
> + status = gs_buf_alloc(&port->port_write_buf);
> spin_lock_irq(&port->port_lock);
>
> if (status) {
> @@ -891,8 +879,7 @@ static int gs_write(struct tty_struct *tty, const unsigned char *buf, int count)
> port->port_num, tty, count);
>
> spin_lock_irqsave(&port->port_lock, flags);
> - if (count)
> - count = gs_buf_put(&port->port_write_buf, buf, count);
> + count = gs_buf_put(&port->port_write_buf, buf, count);
> /* treat count == 0 as flush_chars() */
> if (port->port_usb)
> status = gs_start_tx(port);
> @@ -930,6 +917,10 @@ static void gs_flush_chars(struct tty_struct *tty)
> spin_unlock_irqrestore(&port->port_lock, flags);
> }
>
> +/*
> + * This could be made lock-free, if gs_buf's buf_get and buf_put were
> + * made atomic. Is that worth doing?
> + */
> static int gs_write_room(struct tty_struct *tty)
> {
> struct gs_port *port = tty->driver_data;
> @@ -947,6 +938,7 @@ static int gs_write_room(struct tty_struct *tty)
> return room;
> }
>
> +/* Likewise */
> static int gs_chars_in_buffer(struct tty_struct *tty)
> {
> struct gs_port *port = tty->driver_data;
>
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
