From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C4CAC433FE for ; Mon, 24 Jan 2022 16:38:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0E5EE6B0096; Mon, 24 Jan 2022 11:38:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0700F6B0099; Mon, 24 Jan 2022 11:38:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E52236B009C; Mon, 24 Jan 2022 11:38:17 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0211.hostedemail.com [216.40.44.211]) by kanga.kvack.org (Postfix) with ESMTP id D05B16B0096 for ; Mon, 24 Jan 2022 11:38:17 -0500 (EST) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 8A4C318274766 for ; Mon, 24 Jan 2022 16:38:17 +0000 (UTC) X-FDA: 79065738234.09.5541941 Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by imf08.hostedemail.com (Postfix) with ESMTP id 9AD1816001A for ; Mon, 24 Jan 2022 16:38:09 +0000 (UTC) Received: by mail-yb1-f170.google.com with SMTP id g14so53010946ybs.8 for ; Mon, 24 Jan 2022 08:38:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=J8ycivHdU9v2HmuEv4lTg2qC6izyVod1QqFr0SelWOE=; b=Ji+MSNF4pYwPoh7iPNo0AIs29aypuZNI03FIOxoIf4UlhRewBmOnp7m6hLTM2fEM+E mkTUHB5wtnyvPphdtxKwnjLuTCPiOsrPulAwu4Au1almQbVWdocm98GhXO1+wbbU5m8x 9ny7w1HqLHLWNmkai3OZkesA5H2H+UqACv8C0wxHoKOL8snZTfd56Gvj2qasAyEUm5pa /kNU6cAFW/BkyKA4hpgnyCWxLDwZdjIRIWawvLp0OFHzgwyHXvtBbQZ570v4BcThxJUr +LooAypKttU4Ba8L6ceEjp6oC8P+N0Q+H3NwWo6fz3i2bi/xyuS+Wt4BOfieOZ35cAzb h4Sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=J8ycivHdU9v2HmuEv4lTg2qC6izyVod1QqFr0SelWOE=; b=4TW2PADzuqyNYiPpCzkS5/4WNI5TrEqxpAh4e+jHAlTKEqZpisgjZidem4GsrCIJRh xmnScP+n4Ogk1GgqHl23I/4zpZVs7vy83ZZFL16n47nmqvfwk+iSfBJrmF54P0Pxem0Z Bpo01T5vCHAOR/Lg9XO7zzv8hSJLlybozni5sZBWFAse8zoPPJcJblNEGJax+BDAj79E dKUlyxoIZDdM2pGgc4RB8Iq8uz+Oy+5IDoCYSDZIqGPt9qsALe80UQk5HrT+90zR5MiV Ti7R5Qf6aI2m4Z+NEAKo+jR0RgIH3HxYdWY+vQ6hZhgBaMWcKNfSkI3s/hBxO0WTZaaS VhyQ== X-Gm-Message-State: AOAM530FZXDsdDZV6H1HIZwr8wdnV9I1nXqzNoPKi5EJNEWw2883z3ie 9/74DmotBrTpTipsHiurScfahlcMIfGx6dImUPI2LA== X-Google-Smtp-Source: ABdhPJwdmbYvlu5sk5zS4/vbCTud0ofYtsymijqTtFDv8truJQRJYVBkxNIMGQbbB8TPYAabRLWn4T3853biseIK0RI= X-Received: by 2002:a25:d617:: with SMTP id n23mr25290949ybg.488.1643042288901; Mon, 24 Jan 2022 08:38:08 -0800 (PST) MIME-Version: 1.0 References: <1642649516-15076-1-git-send-email-huangzhaoyang@gmail.com> In-Reply-To: From: Suren Baghdasaryan Date: Mon, 24 Jan 2022 08:37:58 -0800 Message-ID: Subject: Re: [Resend PATCH v3] psi: fix possible trigger missing in the window To: Johannes Weiner Cc: Huangzhaoyang , Peter Zijlstra , Zhaoyang Huang , Ingo Molnar , linux-mm , LKML Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 9AD1816001A X-Stat-Signature: oofy3rhn8cx1ujanqkzhticopxgc1h8w Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Ji+MSNF4; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf08.hostedemail.com: domain of surenb@google.com designates 209.85.219.170 as permitted sender) smtp.mailfrom=surenb@google.com X-HE-Tag: 1643042289-276948 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Jan 24, 2022 at 7:53 AM Johannes Weiner wrote: > > On Thu, Jan 20, 2022 at 11:31:56AM +0800, Huangzhaoyang wrote: > > From: Zhaoyang Huang > > > > When a new threshold breaching stall happens after a psi event was > > generated and within the window duration, the new event is not > > generated because the events are rate-limited to one per window. If > > after that no new stall is recorded then the event will not be > > generated even after rate-limiting duration has passed. This is > > happening because with no new stall, window_update will not be called > > even though threshold was previously breached. To fix this, record > > threshold breaching occurrence and generate the event once window > > duration is passed. > > > > Suggested-by: Suren Baghdasaryan > > Signed-off-by: Zhaoyang Huang > > Good catch. The change makes sense to me. > > However, I had to re-read the discussion to understand *why* > triggering once per window can be a practical problem. Could you > please include the lkmd scenario you mentioned? > > Suren, even though it's your suggested code, can you please also add > ack/review tags? Thanks! Will do as soon as the new version is posted and your comments are addressed. Thanks! > > Some minor inline comments: > > > diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h > > index 0a23300..87b694a 100644 > > --- a/include/linux/psi_types.h > > +++ b/include/linux/psi_types.h > > @@ -132,6 +132,8 @@ struct psi_trigger { > > > > /* Refcounting to prevent premature destruction */ > > struct kref refcount; > > + > > + bool threshold_breach; > > Something like bool pending_event would be more descriptive, IMO. > > Also please remember to add a short comment like we have for the other > struct members. For example: > > /* Deferred event(s) from previous ratelimit window */ > > > @@ -524,24 +524,29 @@ static u64 update_triggers(struct psi_group *group, u64 now) > > */ > > list_for_each_entry(t, &group->triggers, node) { > > u64 growth; > > + bool trigger_stalled = > > + group->polling_total[t->state] != total[t->state]; > > Triggers don't stall, they trigger on stalls. How about this: > > bool new_stall; > u64 growth; > > new_stall = group->polling_total[t->state] != total[t->state]; > > (order local declarations by length, avoid line wraps where possible) > > > - /* Check for stall activity */ > > - if (group->polling_total[t->state] == total[t->state]) > > - continue; > > - > > - /* > > - * Multiple triggers might be looking at the same state, > > - * remember to update group->polling_total[] once we've > > - * been through all of them. Also remember to extend the > > - * polling time if we see new stall activity. > > - */ > > - new_stall = true; > > - > > - /* Calculate growth since last update */ > > - growth = window_update(&t->win, now, total[t->state]); > > - if (growth < t->threshold) > > + /* Check for stall activity or a previous threshold breach */ > > + if (!trigger_stalled && !t->threshold_breach) > > continue; > > This could use a bit more explanation imo: > > /* > * Check for new stall activity, as well as deferred > * events that occurred in the last window after the > * trigger had already fired (we want to ratelimit > * events without dropping any). > */ > if (!new_stall && !t->pending_event) > continue; > > > + if (trigger_stalled) { > > + /* > > + * Multiple triggers might be looking at the same state, > > + * remember to update group->polling_total[] once we've > > + * been through all of them. Also remember to extend the > > + * polling time if we see new stall activity. > > + */ > > + new_stall = true; > > and then rename this flag `update_total'. > > > + /* Calculate growth since last update */ > > + growth = window_update(&t->win, now, total[t->state]); > > + if (growth < t->threshold) > > + continue; > > + > > + t->threshold_breach = true; > > + } > > /* Limit event signaling to once per window */ > > if (now < t->last_event_time + t->win.size) > > continue; > > @@ -550,6 +555,8 @@ static u64 update_triggers(struct psi_group *group, u64 now) > > if (cmpxchg(&t->event, 0, 1) == 0) > > wake_up_interruptible(&t->event_wait); > > t->last_event_time = now; > > + /* Reset threshold breach flag once event got generated */ > > + t->threshold_breach = false; > > } > > > > if (new_stall) > > @@ -1152,6 +1159,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group, > > t->last_event_time = 0; > > init_waitqueue_head(&t->event_wait); > > kref_init(&t->refcount); > > + t->threshold_breach = false; > > > > mutex_lock(&group->trigger_lock); > > Thanks!