virtio: console: Barrier needed before dropping early_put_chars?

virtio: console: Barrier needed before dropping early_put_chars?

[Amit Shah]

Hey Rusty, Christian,

Christoph Hellwig asked why we don't need a barrier before this code in
virtcons_probe():

> +     /* Start using the new console output. */
> +     early_put_chars = NULL;
>       return 0;

Since only s390 uses early_put_chars so far, you'd know why it's not
needed / why we're safe.

Thanks,
		Amit

Re: virtio: console: Barrier needed before dropping early_put_chars?

[Christian Borntraeger]

Am Dienstag 23 Februar 2010 18:10:22 schrieb Amit Shah:
> Hey Rusty, Christian,
> 
> Christoph Hellwig asked why we don't need a barrier before this code in
> virtcons_probe():
> 
> > +     /* Start using the new console output. */
> > +     early_put_chars = NULL;
> >       return 0;
> 
> Since only s390 uses early_put_chars so far, you'd know why it's not
> needed / why we're safe.

lguest is also using early_put_chars. See arch/x86/lguest/boot.c
Anyway I had to look into linux-next to see this code. I guess that is the
version you are talking about. (I remember seeing these patches several month
ago).

So what would a barrier do? Protect against gcc moving the early_put_chars=NULL
before the virtqueue is fully initialized and ready?
In practice this should not be necessary since the last thing probe might do
is add_port which is doing send_control_msg which contains cpu_relax (which
is a barrier). So gcc should not be able to move the early_put_chars = NULL
before the add_port loop.

Anyway, an explicit barrier might be a cleaner and more future proof way of
doing it.

Re: virtio: console: Barrier needed before dropping early_put_chars?

[Rusty Russell]

On Wed, 24 Feb 2010 03:40:22 am Amit Shah wrote:
> Hey Rusty, Christian,
> 
> Christoph Hellwig asked why we don't need a barrier before this code in
> virtcons_probe():
> 
> > +     /* Start using the new console output. */
> > +     early_put_chars = NULL;
> >       return 0;
> 
> Since only s390 uses early_put_chars so far, you'd know why it's not
> needed / why we're safe.

He's right, it's sloppy.

In practice the compiler checks for NULL and reuses the pointer, and we
have no problem if this is used a couple of times after the real console is
live.

The Right Way to do this is a lock in put_chars() and around this assignment.
But do we really want to bother?

Cheers,
Rusty.
-- 
Away travelling 25Feb-26Mar (6 .de + 1 .pl + 17 .lt + 2 .sg)