From: Vitaliy Makkoveev Subject: Re: wsmouse(4) and wstpad: mp-safe filterops To: bluhm@openbsd.org, tech@openbsd.org Date: Thu, 23 Jan 2025 20:59:46 +0300 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 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.