svn commit: r258325 - user/ed/newcons/sys/dev/vt

[Nathan Whitehorn]

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;
>