From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: wsmouse(4) and wstpad: mp-safe filterops
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: bluhm@openbsd.org, tech@openbsd.org
Date: Thu, 23 Jan 2025 20:08:01 +0100

On Thu, 23 Jan 2025 18:59:46 +0100,
Vitaliy Makkoveev <mvs@openbsd.org> wrote:
> 
> On Thu, Jan 23, 2025 at 06:42:37PM +0100, Kirill A. Korinsky wrote:
> > On Thu, 23 Jan 2025 11:14:31 +0100,
> > Vitaliy Makkoveev <mvs@openbsd.org> wrote:
> > >
> > > The last one.
> > >
> > > As in wskbd(4), we don't share `sc_refcnt' with interrupts, so we don't
> > > need to protect it. May be `sc_refcnt' should be replaced with refcnt
> > > structure, may be completely removed - it's not yet clean to me.
> > >
> > > Please note, filt_wseventread() is the only thread that runs
> > > simultaneously with the rest of the wscons(4). This thread only compare
> > > `ws_get' with `ws_put'. wscons(4) drivers only increment `ws_put' index
> > > so at driver side we only need to protect this increment to ensure
> > > `ws_put' is consistent with WSEVENT_QSIZE. I wrapped `ws_put' and
> > > `ws_get' access with `ws_mtx' mutex(9) but this is not required.
> > >
> 
> 
> 
> > > @@ -955,7 +956,10 @@ wsmouse_evq_put(struct evq_access *evq,
> > >  	struct wscons_event *ev;
> > >  	int space;
> > >
> > > +	mtx_enter(&evq->evar->ws_mtx);
> > >  	space = evq->evar->ws_get - evq->put;
> > > +	mtx_leave(&evq->evar->ws_mtx);
> > > +
> > >  	if (space != 1 && space != 1 - WSEVENT_QSIZE) {
> > >  		ev = &evq->evar->ws_q[evq->put++];
> > >  		evq->put %= WSEVENT_QSIZE;
> > 
> > Here and in wsmouse_log_events this block does not smell mp-safe.
> > 
> > Am I wrong? Should I call mtx_leave after the if block?
> > 
> 
> See my diff description. When some variable modification happens with
> two or more locks held, you need only one of them to read-only access.
> 
> This time all except filt_wseventread() serialized with kernel lock.
> filt_wseventread() does read-only access, all `ws_get' and `ws_put'
> modifications happens with both kernel lock and `ws_mtx' mutex(9) held,
> so actually we don't need to take `ws_mtx' in this hunk, kernel lock is
> pretty enough.
> 
> Note, this diff doesn't make ws* devices fully mp-safe, it only makes
> mp-safe polling. I see no reason to introduce useless locks and
> serialization for now. May be I will start doing this in next -current
> cycle.
> 

I see. This, indeed, misleas me.

FWIW you have my OK.

-- 
wbr, Kirill