Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()

Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()

[Greg KH]

On Sat, Oct 09, 2021 at 07:48:28PM +0800, Xianting Tian wrote:
> As well known, hvc backend can register its opertions to hvc backend.
> the operations contain put_chars(), get_chars() and so on.
> 
> Some hvc backend may do dma in its operations. eg, put_chars() of
> virtio-console. But in the code of hvc framework, it may pass DMA
> incapable memory to put_chars() under a specific configuration, which
> is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
> 1, c[] is on stack,
>    hvc_console_print():
> 	char c[N_OUTBUF] __ALIGNED__;
> 	cons_ops[index]->put_chars(vtermnos[index], c, i);
> 2, ch is on stack,
>    static void hvc_poll_put_char(,,char ch)
>    {
> 	struct tty_struct *tty = driver->ttys[0];
> 	struct hvc_struct *hp = tty->driver_data;
> 	int n;
> 
> 	do {
> 		n = hp->ops->put_chars(hp->vtermno, &ch, 1);
> 	} while (n <= 0);
>    }
> 
> Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
> is passed to virtio-console by hvc framework in above code. But I think
> the fix is aggressive, it directly uses kmemdup() to alloc new buffer
> from kmalloc area and do memcpy no matter the memory is in kmalloc area
> or not. But most importantly, it should better be fixed in the hvc
> framework, by changing it to never pass stack memory to the put_chars()
> function in the first place. Otherwise, we still face the same issue if
> a new hvc backend using dma added in the furture.
> 
> In this patch, add 'char cons_outbuf[]' as part of 'struct hvc_struct',
> so hp->cons_outbuf is no longer the stack memory, we can use it in above
> case 1. Add 'char outchar' as part of 'struct hvc_struct', we can use it
> in above case 2. We also add lock for each above buf to protect them
> separately instead of using the global lock of hvc.
> 
> Introduce another array(cons_hvcs[]) for hvc pointers next to the
> cons_ops[] and vtermnos[] arrays. With the array, we can easily find
> hvc's cons_outbuf and its lock.
> 
> With the patch, we can revert the fix c4baad5029.
> 
> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
> Signed-off-by: Shile Zhang <shile.zhang@linux.alibaba.com>
> ---
>  drivers/tty/hvc/hvc_console.c | 37 +++++++++++++++++++++--------------
>  drivers/tty/hvc/hvc_console.h | 24 +++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 5bb8c4e44..4d8f112f2 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -41,16 +41,6 @@
>   */
>  #define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
>  
> -/*
> - * These sizes are most efficient for vio, because they are the
> - * native transfer size. We could make them selectable in the
> - * future to better deal with backends that want other buffer sizes.
> - */
> -#define N_OUTBUF	16
> -#define N_INBUF		16
> -
> -#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
> -

Are you sure this applies on top of patch 1/3 here?

> +/*
> + * These sizes are most efficient for vio, because they are the
> + * native transfer size. We could make them selectable in the
> + * future to better deal with backends that want other buffer sizes.
> + */
> +#define N_OUTBUF	16
> +#define N_INBUF		16
> +
> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))

Again, are you sure this is correct?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()

[Greg KH]

On Sat, Oct 09, 2021 at 07:48:28PM +0800, Xianting Tian wrote:
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -32,13 +32,21 @@
>   */
>  #define HVC_ALLOC_TTY_ADAPTERS	8
>  
> +/*
> + * These sizes are most efficient for vio, because they are the
> + * native transfer size. We could make them selectable in the
> + * future to better deal with backends that want other buffer sizes.
> + */
> +#define N_OUTBUF	16
> +#define N_INBUF		16
> +
> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))

Does this conflict with what is in hvcs.c?

> +
>  struct hvc_struct {
>  	struct tty_port port;
>  	spinlock_t lock;
>  	int index;
>  	int do_wakeup;
> -	char *outbuf;
> -	int outbuf_size;
>  	int n_outbuf;
>  	uint32_t vtermno;
>  	const struct hv_ops *ops;
> @@ -48,6 +56,18 @@ struct hvc_struct {
>  	struct work_struct tty_resize;
>  	struct list_head next;
>  	unsigned long flags;
> +
> +	/* the buf is used in hvc console api for putting chars */
> +	char cons_outbuf[N_OUTBUF] __ALIGNED__;
> +	spinlock_t cons_outbuf_lock;

Did you look at the placement using pahole as to how this structure now
looks?

> +
> +	/* the buf is for putting single char to tty */
> +	char outchar;
> +	spinlock_t outchar_lock;

So you have a lock for a character and a different one for a longer
string?  Why can they not just use the same lock?  Why are 2 needed at
all, can't you just use the first character of cons_outbuf[] instead?
Surely you do not have 2 sends happening at the same time, right?

> +
> +	/* the buf is for putting chars to tty */
> +	int outbuf_size;
> +	char outbuf[0] __ALIGNED__;

I thought we were not allowing [0] anymore in kernel structures?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()

[Greg KH]

On Sat, Oct 09, 2021 at 11:45:23PM +0800, Xianting Tian wrote:
> 
> 在 2021/10/9 下午7:58, Greg KH 写道:
> > Did you look at the placement using pahole as to how this structure now
> > looks?
> 
> thanks for all your commnts. for this one, do you mean I need to remove the
> blank line?  thanks
>

No, I mean to use the tool 'pahole' to see the structure layout that you
just created and determine if it really is the best way to add these new
fields, especially as you are adding huge buffers with odd alignment.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()

[Greg KH]

On Thu, Oct 14, 2021 at 04:34:59PM +0800, Xianting Tian wrote:
> 
> 在 2021/10/10 下午1:33, Greg KH 写道:
> > On Sat, Oct 09, 2021 at 11:45:23PM +0800, Xianting Tian wrote:
> > > 在 2021/10/9 下午7:58, Greg KH 写道:
> > > > Did you look at the placement using pahole as to how this structure now
> > > > looks?
> > > thanks for all your commnts. for this one, do you mean I need to remove the
> > > blank line?  thanks
> > > 
> > No, I mean to use the tool 'pahole' to see the structure layout that you
> > just created and determine if it really is the best way to add these new
> > fields, especially as you are adding huge buffers with odd alignment.
> 
> thanks,
> 
> Based on your comments, I removed 'char outchar',  remian the position of
> 'int outbuf_size' unchanged to keep outbuf_size and lock in the same cache
> line.  Now hvc_struct change as below,
> 
>  struct hvc_struct {
>         struct tty_port port;
>         spinlock_t lock;
>         int index;
>         int do_wakeup;
> -       char *outbuf;
>         int outbuf_size;
>         int n_outbuf;
>         uint32_t vtermno;
> @@ -48,6 +57,16 @@ struct hvc_struct {
>         struct work_struct tty_resize;
>         struct list_head next;
>         unsigned long flags;
> +
> +       /*
> +        * the buf is used in hvc console api for putting chars,
> +        * and also used in hvc_poll_put_char() for putting single char.
> +        */
> +       char cons_outbuf[N_OUTBUF] __ALIGNED__;
> +       spinlock_t cons_outbuf_lock;
> +
> +       /* the buf is used for putting chars to tty */
> +       char outbuf[] __ALIGNED__;
>  };
> 
> pahole for above hvc_struct as below,  is it ok for you?  do we need to pack
> the hole? thanks
> 
> struct hvc_struct {
>     struct tty_port            port;                 /*     0 352 */
>     /* --- cacheline 5 boundary (320 bytes) was 32 bytes ago --- */
>     spinlock_t                 lock;                 /*   352 4 */
>     int                        index;                /*   356 4 */
>     int                        do_wakeup;            /*   360 4 */
>     int                        outbuf_size;          /*   364 4 */
>     int                        n_outbuf;             /*   368 4 */
>     uint32_t                   vtermno;              /*   372 4 */
>     const struct hv_ops  *     ops;                  /*   376 8 */
>     /* --- cacheline 6 boundary (384 bytes) --- */
>     int                        irq_requested;        /*   384 4 */
>     int                        data;                 /*   388 4 */
>     struct winsize             ws;                   /*   392 8 */
>     struct work_struct         tty_resize;           /*   400 32 */
>     struct list_head           next;                 /*   432 16 */
>     /* --- cacheline 7 boundary (448 bytes) --- */
>     long unsigned int          flags;                /*   448 8 */
> 
>     /* XXX 56 bytes hole, try to pack */
> 
>     /* --- cacheline 8 boundary (512 bytes) --- */
>     char                       cons_outbuf[16];      /*   512 16 */
>     spinlock_t                 cons_outbuf_lock;     /*   528 4 */
> 
>     /* XXX 44 bytes hole, try to pack */

Why not move the spinlock up above the cons_outbuf?  Will that not be a
bit better?

Anyway, this is all fine, and much better than before, thanks.

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization