Date: Wed, 20 Nov 2013 10:42:08 -0600 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Aleksandr Rybalko <ray@FreeBSD.org>, src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r258325 - user/ed/newcons/sys/dev/vt Message-ID: <528CE660.5070004@freebsd.org> In-Reply-To: <201311182239.rAIMdYIY042117@svn.freebsd.org> References: <201311182239.rAIMdYIY042117@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
These atomic ops don't have their desired effect. On many platforms,
atomic_set_int()/clear_int() are just a = b, so most of this diff is a
no-op. The larger problem is code like this:
if (vw->vw_flags & VWF_SWWAIT_ACQ) {
atomic_clear_int(&vw->vw_flags, VWF_SWWAIT_ACQ);
This is racy, because the entire check-and-clear operation is not
atomic. You probably want some variation on atomic_cmpset() (in a loop).
Depending on what the rest of the code is doing, you may just need
mutexes to avoid races.
-Nathan
On 11/18/13 16:39, Aleksandr Rybalko wrote:
> Author: ray
> Date: Mon Nov 18 22:39:34 2013
> New Revision: 258325
> URL: http://svnweb.freebsd.org/changeset/base/258325
>
> Log:
> Switch to use atomic ops for VT window flags, because some modifications can
> come from another thread.
>
> Sponsored by: The FreeBSD Foundation
>
> Modified:
> user/ed/newcons/sys/dev/vt/vt_core.c
>
> Modified: user/ed/newcons/sys/dev/vt/vt_core.c
> ==============================================================================
> --- user/ed/newcons/sys/dev/vt/vt_core.c Mon Nov 18 22:37:01 2013 (r258324)
> +++ user/ed/newcons/sys/dev/vt/vt_core.c Mon Nov 18 22:39:34 2013 (r258325)
> @@ -355,7 +355,7 @@ vt_scrollmode_kbdevent(struct vt_window
> /* Turn scrolling off. */
> vt_scroll(vw, 0, VHS_END);
> VTBUF_SLCK_DISABLE(&vw->vw_buf);
> - vw->vw_flags &= ~VWF_SCROLL;
> + atomic_clear_int(&vw->vw_flags, VWF_SCROLL);
> break;
> }
> case FKEY | F(49): /* Home key. */
> @@ -438,11 +438,11 @@ vt_processkey(keyboard_t *kbd, struct vt
> VT_LOCK(vd);
> if (state & SLKED) {
> /* Turn scrolling on. */
> - vw->vw_flags |= VWF_SCROLL;
> + atomic_set_int(&vw->vw_flags, VWF_SCROLL);
> VTBUF_SLCK_ENABLE(&vw->vw_buf);
> } else {
> /* Turn scrolling off. */
> - vw->vw_flags &= ~VWF_SCROLL;
> + atomic_clear_int(&vw->vw_flags, VWF_SCROLL);
> VTBUF_SLCK_DISABLE(&vw->vw_buf);
> vt_scroll(vw, 0, VHS_END);
> }
> @@ -889,12 +889,12 @@ vtterm_cngetc(struct terminal *tm)
> kbdd_ioctl(kbd, KDGKBSTATE, (caddr_t)&state);
> if (state & SLKED) {
> /* Turn scrolling on. */
> - vw->vw_flags |= VWF_SCROLL;
> + atomic_set_int(&vw->vw_flags, VWF_SCROLL);
> VTBUF_SLCK_ENABLE(&vw->vw_buf);
> } else {
> /* Turn scrolling off. */
> vt_scroll(vw, 0, VHS_END);
> - vw->vw_flags &= ~VWF_SCROLL;
> + atomic_clear_int(&vw->vw_flags, VWF_SCROLL);
> VTBUF_SLCK_DISABLE(&vw->vw_buf);
> }
> break;
> @@ -934,9 +934,9 @@ vtterm_opened(struct terminal *tm, int o
> VT_LOCK(vd);
> vd->vd_flags &= ~VDF_SPLASH;
> if (opened)
> - vw->vw_flags |= VWF_OPENED;
> + atomic_set_int(&vw->vw_flags, VWF_OPENED);
> else {
> - vw->vw_flags &= ~VWF_OPENED;
> + atomic_clear_int(&vw->vw_flags, VWF_OPENED);
> /* TODO: finish ACQ/REL */
> }
> VT_UNLOCK(vd);
> @@ -974,7 +974,7 @@ vt_change_font(struct vt_window *vw, str
> VT_UNLOCK(vd);
> return (ENOTTY);
> }
> - vw->vw_flags |= VWF_BUSY;
> + atomic_set_int(&vw->vw_flags, VWF_BUSY);
> VT_UNLOCK(vd);
>
> vt_termsize(vd, vf, &size);
> @@ -997,7 +997,7 @@ vt_change_font(struct vt_window *vw, str
> /* Force a full redraw the next timer tick. */
> if (vd->vd_curwindow == vw)
> vd->vd_flags |= VDF_INVALID;
> - vw->vw_flags &= ~VWF_BUSY;
> + atomic_clear_int(&vw->vw_flags, VWF_BUSY);
> VT_UNLOCK(vd);
> return (0);
> }
> @@ -1034,7 +1034,7 @@ signal_vt_rel(struct vt_window *vw)
> vw->vw_pid = 0;
> return TRUE;
> }
> - vw->vw_flags |= VWF_SWWAIT_REL;
> + atomic_set_int(&vw->vw_flags, VWF_SWWAIT_REL);
> PROC_LOCK(vw->vw_proc);
> kern_psignal(vw->vw_proc, vw->vw_smode.relsig);
> PROC_UNLOCK(vw->vw_proc);
> @@ -1055,7 +1055,7 @@ signal_vt_acq(struct vt_window *vw)
> vw->vw_pid = 0;
> return TRUE;
> }
> - vw->vw_flags |= VWF_SWWAIT_ACQ;
> + atomic_set_int(&vw->vw_flags, VWF_SWWAIT_ACQ);
> PROC_LOCK(vw->vw_proc);
> kern_psignal(vw->vw_proc, vw->vw_smode.acqsig);
> PROC_UNLOCK(vw->vw_proc);
> @@ -1068,7 +1068,7 @@ finish_vt_rel(struct vt_window *vw, int
> {
>
> if (vw->vw_flags & VWF_SWWAIT_REL) {
> - vw->vw_flags &= ~VWF_SWWAIT_REL;
> + atomic_clear_int(&vw->vw_flags, VWF_SWWAIT_REL);
> if (release) {
> callout_drain(&vw->vw_proc_dead_timer);
> vt_late_window_switch(vw->vw_switch_to);
> @@ -1083,7 +1083,7 @@ finish_vt_acq(struct vt_window *vw)
> {
>
> if (vw->vw_flags & VWF_SWWAIT_ACQ) {
> - vw->vw_flags &= ~VWF_SWWAIT_ACQ;
> + atomic_clear_int(&vw->vw_flags, VWF_SWWAIT_ACQ);
> return 0;
> }
> return EINVAL;
> @@ -1392,9 +1392,9 @@ vtterm_ioctl(struct terminal *tm, u_long
> case VT_LOCKSWITCH:
> /* TODO: Check current state, switching can be in progress. */
> if ((*(int *)data) & 0x01)
> - vw->vw_flags |= VWF_VTYLOCK;
> + atomic_set_int(&vw->vw_flags, VWF_VTYLOCK);
> else
> - vw->vw_flags &= ~VWF_VTYLOCK;
> + atomic_clear_int(&vw->vw_flags, VWF_VTYLOCK);
> case VT_OPENQRY: {
> unsigned int i;
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?528CE660.5070004>