From owner-svn-src-head@freebsd.org Wed Nov 18 15:20:36 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 549E146A306; Wed, 18 Nov 2020 15:20:36 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Cbmj01n52z3mNy; Wed, 18 Nov 2020 15:20:36 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x442.google.com with SMTP id u12so2640726wrt.0; Wed, 18 Nov 2020 07:20:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=NgX+vyM2d0eFDionULbFchaZ5NR+NrnZc8rFGiUHIZE=; b=MFXDASXEkYmyaIoe252jI4LnIsvC+biorpL41HE1C6hQKQ+DNwVGAKGsX6E8w9jIwj VAqZ4GZZpOL5/iVNu1us+oeiGxwG2ak4dE3JWTVxW6cl3Ip6aUSHkLi1agIQof1FzmOH nCJoKZ5x7N0NL0BlOGDANrIlj8U1cxDl90allMXrWdnZyTYaHf3kPS7ub/0HiYj7UlkA xy4SDDg6eYGquuIUKrFxMDwo9b0+ksxNr/h/6e0ZTz3Er0IK2J56ail+Z3uiUxRXBcvu qnS8/KxfiHnagNp7Qi2PhS037hrGpkcMmPS55+41qdSh1AmNmAP3EbEOtHrN0fO3AU0T I2mw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=NgX+vyM2d0eFDionULbFchaZ5NR+NrnZc8rFGiUHIZE=; b=IhC+URU7ZgVOpgrm+XqRG+XpuGBlOKw6KBZVFmrBbxkSdGaM3tZSsfs33ao+H1Qq/E /y1FLswuT+CmbJ46iDkej90yfIzMU2x/3DkGMjxBLsNlN6k5vzei+dU9zq4b9u0Nlvi5 m9eFseuxey2XUwraZlMp3HdOdjq9q1mG/2MSleJtn0XNqT8Ja/Iz4McJbOipXganODAB 0ttDMgAMGeKPuEq39ymgYQGv8Q26BvYCL8V1KdUyymVmAyarph4FZxJJdz1FgUCUuH0T QLUPONXlswWp9yh6akfjJF3bakV6W8+/MpIyNSCJFnuLbCdBfc8klEnslpZW74Zp3yZp Ltiw== X-Gm-Message-State: AOAM531rR5snyx0G8p1zFjg0a6dLDAdtYhsBSm9Vobmbxn6eAyk3/eH4 zF4de8Hq/Sw9b4oqIEhpfaj+5wZQLA4IcPOsSeQ= X-Google-Smtp-Source: ABdhPJxfCw1geN9Q+q3K5lTYwgBcYEUuQprM5yFK8wLY/oi43dEJGyObNaUQZ+CDl8nHHG43e6FBqiZKESENME5EsCQ= X-Received: by 2002:a5d:5146:: with SMTP id u6mr5668417wrt.66.1605712835169; Wed, 18 Nov 2020 07:20:35 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:dec7:0:0:0:0:0 with HTTP; Wed, 18 Nov 2020 07:20:34 -0800 (PST) In-Reply-To: References: <202011160309.0AG39JmP067464@repo.freebsd.org> From: Mateusz Guzik Date: Wed, 18 Nov 2020 16:20:34 +0100 Message-ID: Subject: Re: svn commit: r367713 - head/sys/kern To: Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4Cbmj01n52z3mNy X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2020 15:20:36 -0000 On 11/18/20, Konstantin Belousov wrote: > On Tue, Nov 17, 2020 at 03:36:31PM +0100, Mateusz Guzik wrote: >> On 11/17/20, Konstantin Belousov wrote: >> > On Tue, Nov 17, 2020 at 04:15:12AM +0100, Mateusz Guzik wrote: >> >> On 11/17/20, Konstantin Belousov wrote: >> >> > On Mon, Nov 16, 2020 at 03:09:19AM +0000, Mateusz Guzik wrote: >> >> >> Author: mjg >> >> >> Date: Mon Nov 16 03:09:18 2020 >> >> >> New Revision: 367713 >> >> >> URL: https://svnweb.freebsd.org/changeset/base/367713 >> >> >> >> >> >> Log: >> >> >> select: replace reference counting with memory barriers in selfd >> >> >> >> >> >> Refcounting was added to combat a race between selfdfree and >> >> >> doselwakup, >> >> >> but it adds avoidable overhead. >> >> >> >> >> >> selfdfree detects it can free the object by ->sf_si == NULL, thus >> >> >> we >> >> >> can >> >> >> ensure that the condition only holds after all accesses are >> >> >> completed. >> >> >> >> >> >> Modified: >> >> >> head/sys/kern/sys_generic.c >> >> >> >> >> >> Modified: head/sys/kern/sys_generic.c >> >> >> ============================================================================== >> >> >> --- head/sys/kern/sys_generic.c Sun Nov 15 22:49:28 2020 (r367712) >> >> >> +++ head/sys/kern/sys_generic.c Mon Nov 16 03:09:18 2020 (r367713) >> >> >> @@ -156,7 +156,6 @@ struct selfd { >> >> >> struct mtx *sf_mtx; /* Pointer to selinfo mtx. */ >> >> >> struct seltd *sf_td; /* (k) owning seltd. */ >> >> >> void *sf_cookie; /* (k) fd or pollfd. */ >> >> >> - u_int sf_refs; >> >> >> }; >> >> >> >> >> >> MALLOC_DEFINE(M_SELFD, "selfd", "selfd"); >> >> >> @@ -1704,16 +1703,17 @@ static void >> >> >> selfdfree(struct seltd *stp, struct selfd *sfp) >> >> >> { >> >> >> STAILQ_REMOVE(&stp->st_selq, sfp, selfd, sf_link); >> >> >> - if (sfp->sf_si != NULL) { >> >> >> + /* >> >> >> + * Paired with doselwakeup. >> >> >> + */ >> >> >> + if (atomic_load_acq_ptr((uintptr_t *)&sfp->sf_si) != >> >> >> (uintptr_t)NULL) >> >> >> { >> >> > This could be != 0. >> >> > >> >> >> mtx_lock(sfp->sf_mtx); >> >> >> if (sfp->sf_si != NULL) { >> >> >> TAILQ_REMOVE(&sfp->sf_si->si_tdlist, sfp, sf_threads); >> >> >> - refcount_release(&sfp->sf_refs); >> >> >> } >> >> >> mtx_unlock(sfp->sf_mtx); >> >> >> } >> >> >> - if (refcount_release(&sfp->sf_refs)) >> >> >> - free(sfp, M_SELFD); >> >> >> + free(sfp, M_SELFD); >> >> > What guarantees that doselwakeup() finished with sfp ? >> >> > >> >> >> >> Release semantics provided by atomic_store_rel_ptr -- it means the >> >> NULL store is the last access, neither CPU nor the compiler are going >> >> to reorder preceding loads/stores past it. >> > It only guarantees that if we observed NULL as the result of load. >> > >> >> If that did not happen selfdfree takes the lock. If the entry is still >> there, it removes it and doselwakeup will not see it after it takes >> the lock. If the entry is not there, doselwaekup is done with it >> because it released the lock. > > I still do not understand it. selfdfree() indeed takes sf_mtx and rechecks > sf_si. But what prevents doselwakeup() to store NULL into sf_si at any > moment. e.g. after free() is done ? selfdfree() takes seltd mutex, not > selinfo. > That's the same lock. In selrecord: mtxp = sip->si_mtx; if (mtxp == NULL) mtxp = mtx_pool_find(mtxpool_select, sip); /* * Initialize the sfp and queue it in the thread. */ sfp->sf_si = sip; sfp->sf_mtx = mtxp; STAILQ_INSERT_TAIL(&stp->st_selq, sfp, sf_link); /* * Now that we've locked the sip, check for initialization. */ mtx_lock(mtxp); if (sip->si_mtx == NULL) { sip->si_mtx = mtxp; TAILQ_INIT(&sip->si_tdlist); } Then doselwakeup mtx_lock(sip->si_mtx) serializes against mtx_lock(sfp->sf_mtx) -- Mateusz Guzik