Re: [PATCH v8 3/3] console: Add persistent scrollback buffers for all VGA consoles

From: Manuel Schölling
Date: Tue Jan 10 2017 - 12:33:15 EST

Next message: Shuah Khan: "Re: [PATCH 4.9 000/206] 4.9.3-stable review"
Previous message: Ard Biesheuvel: "Re: x86-64: Maintain 16-byte stack alignment"
In reply to: Bartlomiej Zolnierkiewicz: "Re: [PATCH v8 3/3] console: Add persistent scrollback buffers for all VGA consoles"
Next in thread: Manuel SchÃlling: "[PATCH v9 1/4] console: Move scrollback data into its own struct"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Bartlomiej,

First of all thank you for you feedback!

On Tue, 2017-01-10 at 17:44 +0100, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday, January 10, 2017 05:22:22 PM Bartlomiej Zolnierkiewicz
> wrote:
> >
> > Hi,
> >
> > The patchset generally looks fine to me but I have a question
> > regarding new VGACON_SOFT_SCROLLBACK_PERSISTENT config option.
> >
> > Since the code size impact to support the persistent scrollback
> > feature is minimal wouldn't it be better to always include it?
> >
> > The feature would be disabled by default and could be enabled
> > by using the new kernel command line parameter (you may also add
> > a new config option for enabling it by default if desired).
>
> Something like:
>
> #ifdef VGACON_SOFT_SCROLLBACK_PERSISTENT_ENABLE_BY_DEFAULT
> static bool scrollback_persistent = 1;
> #else
> static bool scrollback_persistent;
> #endif
>
> module_param_named(scrollback_persistent, scrollback_persistent,
> bool, 0);
> MODULE_PARM_DESC(scrollback_persistent, "Enable persistent scrollback
> feature");
>
> and then use scrollback_persistent variable in
> vgacon_scrollback_switch()
> to control the actual behavior instead of ifdefs.
Sounds pretty good to me. You are right: The code size impact is rather
small. I will apply your suggestions and send you another patch as soon
as I find the time to implement it (should be any time this week).

Thanks again!

Manuel

> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> > On Thursday, January 05, 2017 12:33:20 PM Manuel SchÃlling wrote:
> > > Add a scrollback buffers for each VGA console. The benefit is
> > > that
> > > the scrollback history is not flushed when switching between
> > > consoles
> > > but is persistent.
> > > The buffers are allocated on demand when a new console is opened.
> > >
> > > This breaks tools like clear_console that rely on flushing the
> > > scrollback history by switching back and forth between consoles
> > > which is why this feature is disabled by default.
> > > Use the escape sequence \e[3J instead for flushing the buffer.
> > >
> > > Signed-off-by: Manuel SchÃlling <manuel.schoelling@xxxxxx>
> > > Reviewed-by: Andrey Utkin <andrey_utkin@xxxxxxxxxxxx>
> > > Tested-by: Andrey Utkin <andrey_utkin@xxxxxxxxxxxx>
> > > Tested-by: Adam Borowski <kilobyte@xxxxxxxxxx>
> > > ---
> > > Âdrivers/video/console/KconfigÂÂ|ÂÂ25 +++++++-
> > > Âdrivers/video/console/vgacon.c | 142 ++++++++++++++++++++++++++-
> > > --------------
> > > Â2 files changed, 111 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/drivers/video/console/Kconfig
> > > b/drivers/video/console/Kconfig
> > > index c3f1fb9ee820..f500e58f7636 100644
> > > --- a/drivers/video/console/Kconfig
> > > +++ b/drivers/video/console/Kconfig
> > > @@ -43,9 +43,28 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> > > ÂÂÂÂÂÂÂÂrange 1 1024
> > > ÂÂÂÂÂÂÂÂdefault "64"
> > > ÂÂÂÂÂÂÂÂhelp
> > > -ÂÂÂÂÂÂÂÂÂEnter the amount of System RAM to allocate for the
> > > scrollback
> > > - Âbuffer.ÂÂEach 64KB will give you approximately 16 80x25
> > > - Âscreenfuls of scrollback buffer
> > > + ÂÂEnter the amount of System RAM to allocate for
> > > scrollback
> > > + ÂÂbuffers of VGA consoles. Each 64KB will give you
> > > approximately
> > > + ÂÂ16 80x25 screenfuls of scrollback buffer.
> > > +
> > > +config VGACON_SOFT_SCROLLBACK_PERSISTENT
> > > + bool "Persistent Scrollback History for each console"
> > > + depends on VGACON_SOFT_SCROLLBACK
> > > + default n
> > > + help
> > > + ÂÂSay Y here if the scrollback history should persist
> > > when switching
> > > + ÂÂbetween consoles. Otherwise, the scrollback history
> > > will be flushed
> > > + ÂÂeach time the console is switched.
> > > +
> > > + ÂÂThis feature might break your tool of choice to flush
> > > the scrollback
> > > + ÂÂbuffer, e.g. clear(1) will work fine but Debian's
> > > clear_console(1)
> > > + ÂÂwill be broken, which might cause security issues.
> > > + ÂÂYou can use the escape sequence \e[3J instead if this
> > > feature is
> > > + ÂÂactivated.
> > > +
> > > + ÂÂNote that a buffer of VGACON_SOFT_SCROLLBACK_SIZE is
> > > taken for each
> > > + ÂÂcreated tty device.
> > > + ÂÂSo if you use a RAM-constrained system, say N here.
> > > Â
> > > Âconfig MDA_CONSOLE
> > > Â depends on !M68K && !PARISC && ISA
> > > diff --git a/drivers/video/console/vgacon.c
> > > b/drivers/video/console/vgacon.c
> > > index 9a7c2bbc5326..ca23d222e029 100644
> > > --- a/drivers/video/console/vgacon.c
> > > +++ b/drivers/video/console/vgacon.c
> > > @@ -162,7 +162,7 @@ static inline void vga_set_mem_top(struct
> > > vc_data *c)
> > > Â
> > > Â#ifdef CONFIG_VGACON_SOFT_SCROLLBACK
> > > Â/* software scrollback */
> > > -static struct vgacon_scrollback_info {
> > > +struct vgacon_scrollback_info {
> > > Â void *data;
> > > Â int tail;
> > > Â int size;
> > > @@ -171,74 +171,110 @@ static struct vgacon_scrollback_info {
> > > Â int cur;
> > > Â int save;
> > > Â int restore;
> > > -} vgacon_scrollback;
> > > +};
> > > +
> > > +static struct vgacon_scrollback_info *vgacon_scrollback_cur;
> > > +#ifdef CONFIG_VGACON_SOFT_SCROLLBACK_PERSISTENT
> > > +static struct vgacon_scrollback_info
> > > vgacon_scrollbacks[MAX_NR_CONSOLES];
> > > +#else
> > > +static struct vgacon_scrollback_info vgacon_scrollbacks[1];
> > > +#endif
> > > Â
> > > -static void vgacon_scrollback_reset(size_t reset_size)
> > > +static void vgacon_scrollback_reset(int vc_num, size_t
> > > reset_size)
> > > Â{
> > > - if (vgacon_scrollback.data && reset_size > 0)
> > > - memset(vgacon_scrollback.data, 0, reset_size);
> > > + struct vgacon_scrollback_info *scrollback =
> > > &vgacon_scrollbacks[vc_num];
> > > +
> > > + if (scrollback->data && reset_size > 0)
> > > + memset(scrollback->data, 0, reset_size);
> > > Â
> > > - vgacon_scrollback.cntÂÂ= 0;
> > > - vgacon_scrollback.tail = 0;
> > > - vgacon_scrollback.curÂÂ= 0;
> > > + scrollback->cntÂÂ= 0;
> > > + scrollback->tail = 0;
> > > + scrollback->curÂÂ= 0;
> > > Â}
> > > Â
> > > -static void vgacon_scrollback_init(int pitch)
> > > +static void vgacon_scrollback_init(int vc_num)
> > > Â{
> > > - int rows = CONFIG_VGACON_SOFT_SCROLLBACK_SIZE *
> > > 1024/pitch;
> > > -
> > > - if (vgacon_scrollback.data) {
> > > - vgacon_scrollback.cntÂÂ= 0;
> > > - vgacon_scrollback.tail = 0;
> > > - vgacon_scrollback.curÂÂ= 0;
> > > - vgacon_scrollback.rows = rows - 1;
> > > - vgacon_scrollback.size = rows * pitch;
> > > + int pitch = vga_video_num_columns * 2;
> > > + size_t size = CONFIG_VGACON_SOFT_SCROLLBACK_SIZE * 1024;
> > > + int rows = size / pitch;
> > > + void *data;
> > > +
> > > + data = kmalloc_array(CONFIG_VGACON_SOFT_SCROLLBACK_SIZE,
> > > 1024,
> > > + ÂÂÂÂÂGFP_NOWAIT);
> > > +
> > > + vgacon_scrollbacks[vc_num].data = data;
> > > + vgacon_scrollback_cur = &vgacon_scrollbacks[vc_num];
> > > +
> > > + vgacon_scrollback_cur->rows = rows - 1;
> > > + vgacon_scrollback_cur->size = rows * pitch;
> > > +
> > > + vgacon_scrollback_reset(vc_num, size);
> > > +}
> > > +
> > > +static void vgacon_scrollback_switch(int vc_num)
> > > +{
> > > +#ifndef CONFIG_VGACON_SOFT_SCROLLBACK_PERSISTENT
> > > + vc_num = 0;
> > > +#endif
> > > +
> > > + if (!vgacon_scrollbacks[vc_num].data) {
> > > + vgacon_scrollback_init(vc_num);
> > > + } else {
> > > +#ifdef CONFIG_VGACON_SOFT_SCROLLBACK_PERSISTENT
> > > + vgacon_scrollback_cur =
> > > &vgacon_scrollbacks[vc_num];
> > > +#else
> > > + size_t size = CONFIG_VGACON_SOFT_SCROLLBACK_SIZE
> > > * 1024;
> > > +
> > > + vgacon_scrollback_reset(vc_num, size);
> > > +#endif
> > > Â }
> > > Â}
> > > Â
> > > Âstatic void vgacon_scrollback_startup(void)
> > > Â{
> > > - vgacon_scrollback.data =
> > > kcalloc(CONFIG_VGACON_SOFT_SCROLLBACK_SIZE,
> > > - 1024, GFP_NOWAIT);
> > > - vgacon_scrollback_init(vga_video_num_columns * 2);
> > > + vgacon_scrollback_cur = &vgacon_scrollbacks[0];
> > > + vgacon_scrollback_init(0);
> > > Â}
> > > Â
> > > Âstatic void vgacon_scrollback_update(struct vc_data *c, int t,
> > > int count)
> > > Â{
> > > Â void *p;
> > > Â
> > > - if (!vgacon_scrollback.size || c->vc_num != fg_console)
> > > + if (!vgacon_scrollback_cur->data ||
> > > !vgacon_scrollback_cur->size ||
> > > + ÂÂÂÂc->vc_num != fg_console)
> > > Â return;
> > > Â
> > > Â p = (void *) (c->vc_origin + t * c->vc_size_row);
> > > Â
> > > Â while (count--) {
> > > - scr_memcpyw(vgacon_scrollback.data +
> > > vgacon_scrollback.tail,
> > > + scr_memcpyw(vgacon_scrollback_cur->data +
> > > + ÂÂÂÂvgacon_scrollback_cur->tail,
> > > Â ÂÂÂÂp, c->vc_size_row);
> > > - vgacon_scrollback.cnt++;
> > > +
> > > + vgacon_scrollback_cur->cnt++;
> > > Â p += c->vc_size_row;
> > > - vgacon_scrollback.tail += c->vc_size_row;
> > > + vgacon_scrollback_cur->tail += c->vc_size_row;
> > > Â
> > > - if (vgacon_scrollback.tail >=
> > > vgacon_scrollback.size)
> > > - vgacon_scrollback.tail = 0;
> > > + if (vgacon_scrollback_cur->tail >=
> > > vgacon_scrollback_cur->size)
> > > + vgacon_scrollback_cur->tail = 0;
> > > Â
> > > - if (vgacon_scrollback.cnt >
> > > vgacon_scrollback.rows)
> > > - vgacon_scrollback.cnt =
> > > vgacon_scrollback.rows;
> > > + if (vgacon_scrollback_cur->cnt >
> > > vgacon_scrollback_cur->rows)
> > > + vgacon_scrollback_cur->cnt =
> > > vgacon_scrollback_cur->rows;
> > > Â
> > > - vgacon_scrollback.cur = vgacon_scrollback.cnt;
> > > + vgacon_scrollback_cur->cur =
> > > vgacon_scrollback_cur->cnt;
> > > Â }
> > > Â}
> > > Â
> > > Âstatic void vgacon_restore_screen(struct vc_data *c)
> > > Â{
> > > - vgacon_scrollback.save = 0;
> > > + vgacon_scrollback_cur->save = 0;
> > > Â
> > > - if (!vga_is_gfx && !vgacon_scrollback.restore) {
> > > + if (!vga_is_gfx && !vgacon_scrollback_cur->restore) {
> > > Â scr_memcpyw((u16 *) c->vc_origin, (u16 *) c-
> > > >vc_screenbuf,
> > > Â ÂÂÂÂc->vc_screenbuf_size > vga_vram_size
> > > ?
> > > Â ÂÂÂÂvga_vram_size : c-
> > > >vc_screenbuf_size);
> > > - vgacon_scrollback.restore = 1;
> > > - vgacon_scrollback.cur = vgacon_scrollback.cnt;
> > > + vgacon_scrollback_cur->restore = 1;
> > > + vgacon_scrollback_cur->cur =
> > > vgacon_scrollback_cur->cnt;
> > > Â }
> > > Â}
> > > Â
> > > @@ -252,41 +288,41 @@ static void vgacon_scrolldelta(struct
> > > vc_data *c, int lines)
> > > Â return;
> > > Â }
> > > Â
> > > - if (!vgacon_scrollback.data)
> > > + if (!vgacon_scrollback_cur->data)
> > > Â return;
> > > Â
> > > - if (!vgacon_scrollback.save) {
> > > + if (!vgacon_scrollback_cur->save) {
> > > Â vgacon_cursor(c, CM_ERASE);
> > > Â vgacon_save_screen(c);
> > > - vgacon_scrollback.save = 1;
> > > + vgacon_scrollback_cur->save = 1;
> > > Â }
> > > Â
> > > - vgacon_scrollback.restore = 0;
> > > - start = vgacon_scrollback.cur + lines;
> > > + vgacon_scrollback_cur->restore = 0;
> > > + start = vgacon_scrollback_cur->cur + lines;
> > > Â end = start + abs(lines);
> > > Â
> > > Â if (start < 0)
> > > Â start = 0;
> > > Â
> > > - if (start > vgacon_scrollback.cnt)
> > > - start = vgacon_scrollback.cnt;
> > > + if (start > vgacon_scrollback_cur->cnt)
> > > + start = vgacon_scrollback_cur->cnt;
> > > Â
> > > Â if (end < 0)
> > > Â end = 0;
> > > Â
> > > - if (end > vgacon_scrollback.cnt)
> > > - end = vgacon_scrollback.cnt;
> > > + if (end > vgacon_scrollback_cur->cnt)
> > > + end = vgacon_scrollback_cur->cnt;
> > > Â
> > > - vgacon_scrollback.cur = start;
> > > + vgacon_scrollback_cur->cur = start;
> > > Â count = end - start;
> > > - soff = vgacon_scrollback.tail - ((vgacon_scrollback.cnt
> > > - end) *
> > > - Âc->vc_size_row);
> > > + soff = vgacon_scrollback_cur->tail -
> > > + ((vgacon_scrollback_cur->cnt - end) * c-
> > > >vc_size_row);
> > > Â soff -= count * c->vc_size_row;
> > > Â
> > > Â if (soff < 0)
> > > - soff += vgacon_scrollback.size;
> > > + soff += vgacon_scrollback_cur->size;
> > > Â
> > > - count = vgacon_scrollback.cnt - start;
> > > + count = vgacon_scrollback_cur->cnt - start;
> > > Â
> > > Â if (count > c->vc_rows)
> > > Â count = c->vc_rows;
> > > @@ -300,13 +336,13 @@ static void vgacon_scrolldelta(struct
> > > vc_data *c, int lines)
> > > Â
> > > Â count *= c->vc_size_row;
> > > Â /* how much memory to end of buffer left? */
> > > - copysize = min(count, vgacon_scrollback.size -
> > > soff);
> > > - scr_memcpyw(d, vgacon_scrollback.data + soff,
> > > copysize);
> > > + copysize = min(count, vgacon_scrollback_cur-
> > > >size - soff);
> > > + scr_memcpyw(d, vgacon_scrollback_cur->data +
> > > soff, copysize);
> > > Â d += copysize;
> > > Â count -= copysize;
> > > Â
> > > Â if (count) {
> > > - scr_memcpyw(d, vgacon_scrollback.data,
> > > count);
> > > + scr_memcpyw(d, vgacon_scrollback_cur-
> > > >data, count);
> > > Â d += count;
> > > Â }
> > > Â
> > > @@ -320,13 +356,13 @@ static void vgacon_flush_scrollback(struct
> > > vc_data *c)
> > > Â{
> > > Â size_t size = CONFIG_VGACON_SOFT_SCROLLBACK_SIZE * 1024;
> > > Â
> > > - if (c->vc_num == fg_console)
> > > - vgacon_scrollback_reset(size);
> > > + vgacon_scrollback_reset(c->vc_num, size);
> > > Â}
> > > Â#else
> > > Â#define vgacon_scrollback_startup(...) do { } while (0)
> > > Â#define vgacon_scrollback_init(...)ÂÂÂÂdo { } while (0)
> > > Â#define vgacon_scrollback_update(...)ÂÂdo { } while (0)
> > > +#define vgacon_scrollback_switch(...)ÂÂdo { } while (0)
> > > Â
> > > Âstatic void vgacon_restore_screen(struct vc_data *c)
> > > Â{
> > > @@ -805,7 +841,7 @@ static int vgacon_switch(struct vc_data *c)
> > > Â vgacon_doresize(c, c->vc_cols, c-
> > > >vc_rows);
> > > Â }
> > > Â
> > > - vgacon_scrollback_init(c->vc_size_row);
> > > + vgacon_scrollback_switch(c->vc_num);
> > > Â return 0; /* Redrawing not needed */
> > > Â}
>
>

Next message: Shuah Khan: "Re: [PATCH 4.9 000/206] 4.9.3-stable review"
Previous message: Ard Biesheuvel: "Re: x86-64: Maintain 16-byte stack alignment"
In reply to: Bartlomiej Zolnierkiewicz: "Re: [PATCH v8 3/3] console: Add persistent scrollback buffers for all VGA consoles"
Next in thread: Manuel SchÃlling: "[PATCH v9 1/4] console: Move scrollback data into its own struct"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]