Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752757AbdHNJpv (ORCPT ); Mon, 14 Aug 2017 05:45:51 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:45118 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752521AbdHNJpu (ORCPT ); Mon, 14 Aug 2017 05:45:50 -0400 Date: Mon, 14 Aug 2017 11:45:45 +0200 (CEST) From: Thomas Gleixner To: Vikas Shivappa cc: vikas.shivappa@intel.com, x86@kernel.org, linux-kernel@vger.kernel.org, hpa@zytor.com, peterz@infradead.org, ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com, eranian@google.com, davidcc@google.com, ak@linux.intel.com, sai.praneeth.prakhya@intel.com Subject: Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing In-Reply-To: <1502304395-7166-4-git-send-email-vikas.shivappa@linux.intel.com> Message-ID: References: <1502304395-7166-1-git-send-email-vikas.shivappa@linux.intel.com> <1502304395-7166-4-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1747 Lines: 60 On Wed, 9 Aug 2017, Vikas Shivappa wrote: > @@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) > GFP_KERNEL); > if (!d->rmid_busy_llc) > return -ENOMEM; > + INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo); > + if (has_busy_rmid(r, d)) > + cqm_setup_limbo_handler(d); This is beyond silly. d->rmid_busy_llc is allocated a few lines above. How would a bit be set here? > } > if (is_mbm_total_enabled()) { > tsize = sizeof(*d->mbm_total); > @@ -536,11 +539,25 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r) > list_del(&d->list); > if (is_mbm_enabled()) > cancel_delayed_work(&d->mbm_over); > + > + if (is_llc_occupancy_enabled() && > + has_busy_rmid(r, d)) What is that line break helping here and why can't you just unconditionally cancel the work? > + cancel_delayed_work(&d->cqm_limbo); > + > kfree(d); > - } else if (r == &rdt_resources_all[RDT_RESOURCE_L3] && > - cpu == d->mbm_work_cpu && is_mbm_enabled()) { > - cancel_delayed_work(&d->mbm_over); > - mbm_setup_overflow_handler(d); > + return; > + } > + > + if (r == &rdt_resources_all[RDT_RESOURCE_L3]) { > + if (is_mbm_enabled() && cpu == d->mbm_work_cpu) { > + cancel_delayed_work(&d->mbm_over); > + mbm_setup_overflow_handler(d); I think this is the wrong approach. If the timer is about to fire you essentially double the interval. So you better flush the work, which will reschedule it if needed. > + } > + if (is_llc_occupancy_enabled() && cpu == d->mbm_work_cpu && That want's to be d->cbm_work_cpu, right? > + has_busy_rmid(r, d)) { > + cancel_delayed_work(&d->cqm_limbo); > + cqm_setup_limbo_handler(d); See above. Thanks, tglx