From owner-dev-commits-src-all@freebsd.org Mon Sep 13 08:01:43 2021 Return-Path: Delivered-To: dev-commits-src-all@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 5D14367752A; Mon, 13 Sep 2021 08:01:43 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) (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 4H7Jpb1WBdz4YQj; Mon, 13 Sep 2021 08:01:43 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lf1-x12a.google.com with SMTP id h16so19229963lfk.10; Mon, 13 Sep 2021 01:01:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=WxhzR+ifsHB2h6NyocYKbhdgVzU8llJQ+Y24f2h6pH4=; b=AbBrtdIA1xoAB5OxFLITaBTi5a6SmalHEKUVeG2270RDwiD+TqJhlS5M6JWL/nEeiD IyIOQ7z1hxnQEJMe/noVlQMA1XiViyNnxWaOUF8iiZHWaF+FjFlNYQRyjI6JCZB2hVWQ XPSFGkATEmy6iiZkGbsilzzfxLChvUuF/f6Kclc+UijjPNdnK6kqgAhfa6wsiJbaF5aZ YvuRammobK7RWFU/jyThd7dwapvvhHROLDFwchKDAJp5kr16Ux6Na+MAMuvhCUuQvtOK FCuuJnWDZHGFIgCc3SE/4o8sH/0JnzK1kRlz4mWYXvxHT52PTj2WIp6l849OX73tdQDg eyJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=WxhzR+ifsHB2h6NyocYKbhdgVzU8llJQ+Y24f2h6pH4=; b=w8AEFoqTneePDJMR9leP3yi3SWCHKypno4qIjcAYoEGPSsLAc3Xi1VBDR9EM9cEa/C VAgBzq78KkXaK8bazRdYt8OlFJfbKktauHScrNW9rPqezMDuFwDd2JA5w034ZNvF8KF1 FEOY7AI9JFoV2jKiz90StFL/eAOfMtYJvO/XwWrROBkvvt+8bZhv1mGbO5XTm6ld4VAZ TZrzFtSo7Jw8TXDAUZsvlkAYEHo03H9evbwH6cu9G0ZSTaEg8w31ctlQhp+dOOS0i6Zo HHJgtIHL9GTDTVKoK7r4F3XspofZDn28DAWdoBLN30bfDbx5tPdza0kHUy0AUCGEND2g 2puQ== X-Gm-Message-State: AOAM532tBIauOr2AgDFO7lfM7VPjLg7EdDZwu9HMMkbk+GHa7vwPM6Fg vtfbtDVHWqwnxssTor5/t7cWQgerAuJdN4v+HGuYb7h/ X-Google-Smtp-Source: ABdhPJxt+3Kwn4jnKWza/PUdusakBPWE8sHsTcZoHk+9dPHJ0BEa3N2tJqE/zWMtWa92DjeXs1dy+gkC+SEUI/0YUro= X-Received: by 2002:a05:6512:1296:: with SMTP id u22mr7876016lfs.650.1631520101842; Mon, 13 Sep 2021 01:01:41 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:6f04:0:0:0:0:0 with HTTP; Mon, 13 Sep 2021 01:01:41 -0700 (PDT) In-Reply-To: <202109130411.18D4BUs3004805@gitrepo.freebsd.org> References: <202109130411.18D4BUs3004805@gitrepo.freebsd.org> From: Mateusz Guzik Date: Mon, 13 Sep 2021 10:01:41 +0200 Message-ID: Subject: Re: git: 6fa041d7f125 - main - Measure latency of PMC interruptions To: Wojciech Macek Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4H7Jpb1WBdz4YQj X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Sep 2021 08:01:43 -0000 On 9/13/21, Wojciech Macek wrote: > The branch main has been updated by wma: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=6fa041d7f125db65f400af7f520a41ff78d19cd7 > > commit 6fa041d7f125db65f400af7f520a41ff78d19cd7 > Author: Wojciech Macek > AuthorDate: 2021-09-13 04:08:32 +0000 > Commit: Wojciech Macek > CommitDate: 2021-09-13 04:08:32 +0000 > > Measure latency of PMC interruptions > > Add HWPMC events to measure latency. > Provide sysctl to choose the number of outstanding events which > trigger HWPMC event. > > Obtained from: Semihalf > Sponsored by: Stormshield > Differential revision: https://reviews.freebsd.org/D31283 > --- > sys/kern/kern_intr.c | 58 > ++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 52 insertions(+), 6 deletions(-) > > diff --git a/sys/kern/kern_intr.c b/sys/kern/kern_intr.c > index 1660414a50ef..19401877dfbd 100644 > --- a/sys/kern/kern_intr.c > +++ b/sys/kern/kern_intr.c > @@ -30,6 +30,7 @@ > __FBSDID("$FreeBSD$"); > > #include "opt_ddb.h" > +#include "opt_hwpmc_hooks.h" > #include "opt_kstack_usage_prof.h" > > #include > @@ -75,6 +76,7 @@ struct intr_thread { > struct thread *it_thread; /* Kernel thread. */ > int it_flags; /* (j) IT_* flags. */ > int it_need; /* Needs service. */ > + int it_waiting; /* Waiting in the runq. */ > }; > > /* Interrupt thread flags kept in it_flags */ > @@ -100,13 +102,19 @@ SYSCTL_INT(_hw, OID_AUTO, intr_storm_threshold, > CTLFLAG_RWTUN, > static int intr_epoch_batch = 1000; > SYSCTL_INT(_hw, OID_AUTO, intr_epoch_batch, CTLFLAG_RWTUN, > &intr_epoch_batch, > 0, "Maximum interrupt handler executions without re-entering > epoch(9)"); > +#ifdef HWPMC_HOOKS > +static int intr_hwpmc_waiting_report_threshold = 1; > +SYSCTL_INT(_hw, OID_AUTO, intr_hwpmc_waiting_report_threshold, > CTLFLAG_RWTUN, > + &intr_hwpmc_waiting_report_threshold, 1, > + "Threshold for reporting number of events in a workq"); > +#endif > static TAILQ_HEAD(, intr_event) event_list = > TAILQ_HEAD_INITIALIZER(event_list); > static struct mtx event_lock; > MTX_SYSINIT(intr_event_list, &event_lock, "intr event list", MTX_DEF); > > static void intr_event_update(struct intr_event *ie); > -static int intr_event_schedule_thread(struct intr_event *ie); > +static int intr_event_schedule_thread(struct intr_event *ie, struct > trapframe *frame); > static struct intr_thread *ithread_create(const char *name); > static void ithread_destroy(struct intr_thread *ithread); > static void ithread_execute_handlers(struct proc *p, > @@ -115,6 +123,16 @@ static void ithread_loop(void *); > static void ithread_update(struct intr_thread *ithd); > static void start_softintr(void *); > > +#ifdef HWPMC_HOOKS > +#include > +PMC_SOFT_DEFINE( , , intr, all); > +PMC_SOFT_DEFINE( , , intr, ithread); > +PMC_SOFT_DEFINE( , , intr, filter); > +PMC_SOFT_DEFINE( , , intr, stray); > +PMC_SOFT_DEFINE( , , intr, schedule); > +PMC_SOFT_DEFINE( , , intr, waiting); > +#endif > + > /* Map an interrupt type to an ithread priority. */ > u_char > intr_priority(enum intr_type flags) > @@ -773,7 +791,7 @@ intr_handler_barrier(struct intr_handler *handler) > } > if ((handler->ih_flags & IH_CHANGED) == 0) { > handler->ih_flags |= IH_CHANGED; > - intr_event_schedule_thread(ie); > + intr_event_schedule_thread(ie, NULL); > } > while ((handler->ih_flags & IH_CHANGED) != 0) > msleep(handler, &ie->ie_lock, 0, "ih_barr", 0); > @@ -872,7 +890,7 @@ intr_event_remove_handler(void *cookie) > KASSERT((handler->ih_flags & IH_DEAD) == 0, > ("duplicate handle remove")); > handler->ih_flags |= IH_DEAD; > - intr_event_schedule_thread(ie); > + intr_event_schedule_thread(ie, NULL); > while (handler->ih_flags & IH_DEAD) > msleep(handler, &ie->ie_lock, 0, "iev_rmh", 0); > intr_event_update(ie); > @@ -944,7 +962,7 @@ intr_event_resume_handler(void *cookie) > } > > static int > -intr_event_schedule_thread(struct intr_event *ie) > +intr_event_schedule_thread(struct intr_event *ie, struct trapframe *frame) > { > struct intr_entropy entropy; > struct intr_thread *it; > @@ -986,11 +1004,28 @@ intr_event_schedule_thread(struct intr_event *ie) > atomic_store_rel_int(&it->it_need, 1); > thread_lock(td); > if (TD_AWAITING_INTR(td)) { > +#ifdef HWPMC_HOOKS > + atomic_set_int(&it->it_waiting, 0); atomic_set does not assign the passed value, it or's it. > + if (frame != NULL) > + PMC_SOFT_CALL_TF( , , intr, schedule, frame); > + else > + PMC_SOFT_CALL( , , intr, schedule); > +#endif > CTR3(KTR_INTR, "%s: schedule pid %d (%s)", __func__, td->td_proc->p_pid, > td->td_name); > TD_CLR_IWAIT(td); > sched_add(td, SRQ_INTR); > } else { > +#ifdef HWPMC_HOOKS > + atomic_add_int(&it->it_waiting, 1); Why is this using atomics to begin with? To my reading the field is de facto protected by thread lock, so you can just it->it_waiting++ > + > + if (atomic_load_int(&it->it_waiting) >= > intr_hwpmc_waiting_report_threshold) { The option is enabled by default but hwpmc itself is rarely loaded. Then this avoidable loads intr_hwpmc_waiting_report_threshold to begin with. Instead you can do something in the lines: #define PMC_HOOK_INSTALLED_ANY() __predict_false(pmc_hook != NULL) and use that to guard the other branches. Later on this will be hot patchable so it wont be a problem by default. > + if (frame != NULL) > + PMC_SOFT_CALL_TF( , , intr, waiting, frame); > + else > + PMC_SOFT_CALL( , , intr, waiting); > + } > +#endif > CTR5(KTR_INTR, "%s: pid %d (%s): it_need %d, state %d", > __func__, td->td_proc->p_pid, td->td_name, it->it_need, > TD_GET_STATE(td)); > thread_unlock(td); > @@ -1083,7 +1118,7 @@ swi_sched(void *cookie, int flags) > #endif > } else { > VM_CNT_INC(v_soft); > - error = intr_event_schedule_thread(ie); > + error = intr_event_schedule_thread(ie, NULL); > KASSERT(error == 0, ("stray software interrupt")); > } > } > @@ -1374,12 +1409,23 @@ intr_event_handle(struct intr_event *ie, struct > trapframe *frame) > ret = ih->ih_filter(frame); > else > ret = ih->ih_filter(ih->ih_argument); > +#ifdef HWPMC_HOOKS > + PMC_SOFT_CALL_TF( , , intr, all, frame); > +#endif > KASSERT(ret == FILTER_STRAY || > ((ret & (FILTER_SCHEDULE_THREAD | FILTER_HANDLED)) != 0 && > (ret & ~(FILTER_SCHEDULE_THREAD | FILTER_HANDLED)) == 0), > ("%s: incorrect return value %#x from %s", __func__, ret, > ih->ih_name)); > filter = filter || ret == FILTER_HANDLED; > +#ifdef HWPMC_HOOKS > + if (ret & FILTER_SCHEDULE_THREAD) > + PMC_SOFT_CALL_TF( , , intr, ithread, frame); > + else if (ret & FILTER_HANDLED) > + PMC_SOFT_CALL_TF( , , intr, filter, frame); > + else if (ret == FILTER_STRAY) > + PMC_SOFT_CALL_TF( , , intr, stray, frame); > +#endif > > /* > * Wrapper handler special handling: > @@ -1416,7 +1462,7 @@ intr_event_handle(struct intr_event *ie, struct > trapframe *frame) > if (thread) { > int error __unused; > > - error = intr_event_schedule_thread(ie); > + error = intr_event_schedule_thread(ie, frame); > KASSERT(error == 0, ("bad stray interrupt")); > } > critical_exit(); > -- Mateusz Guzik