Date: Tue, 18 Aug 2015 18:09:01 +0100
From: Matt Fleming <matt@codeblueprint.co.uk>
To: Kanaka Juvva <kanaka.d.juvva@linux.intel.com>
Cc: kanaka.d.juvva@intel.com, glenn.p.williamson@intel.com,
        matt.fleming@intel.com, will.auld@intel.com, andi@firstfloor.org,
        linux-kernel@vger.kernel.org, tony.luck@intel.com,
        peterz@infradead.org, tglx@linutronix.de, tj@kernel.org,
        x86@kernel.org, mingo@redhat.com, hpa@zytor.com,
        vikas.shivappa@intel.com
Subject: Re: [PATCH v3 2/2] perf,x86: skip intel_cqm_stable if CMT is not
 present in a CPU model
Message-ID: <20150818170900.GG3233@codeblueprint.co.uk>
References: <1439019379-10025-1-git-send-email-kanaka.d.juvva@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1439019379-10025-1-git-send-email-kanaka.d.juvva@linux.intel.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4584
Lines: 127

On Sat, 08 Aug, at 12:36:19AM, Kanaka Juvva wrote:
> CMT and MBM are complementary technologies. One technology doesn't
> imply the other technology. If CMT is not present in your CPU model
> intel_cqm_stable() won't be called when computing a free RMID. This
> is because, LLC_OCCUPANCY reading in this case doesn't apply and
> shouldn't be used a criteria for freeing or picking an RMID.

This could do with some additional information.

The main point is that when you re-use an RMID for reading MBM values
that RMID doesn't "remember" the last MBM value it read, they're
instantenous and "non-sticky", so you can skip the recycling. That's
not the case for reading CMT values since old lines in the cache are
still tagged with the RMID you want to re-use and they will be
included the CMT value for that RMID until they're naturally evicted
from the cache.

> Signed-off-by: Kanaka Juvva <kanaka.d.juvva@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/perf_event_intel_cqm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_cqm.c b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> index 63eb68b..7aa3bc0 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> @@ -125,6 +125,7 @@ struct cqm_rmid_entry {
>  	enum rmid_recycle_state state;
>  	struct list_head list;
>  	unsigned long queue_time;
> +	bool config;
>  };
  
This isn't the best field name since 'config' doesn't explain that "We
don't need to stabilize this RMID because it's only been used for
reading MBM values". What about 'needs_stabilizing'?

>  /*
> @@ -232,6 +233,7 @@ static int intel_cqm_setup_rmid_cache(void)
>  
>  		INIT_LIST_HEAD(&entry->list);
>  		entry->rmid = r;
> +		entry->config = false;
>  		cqm_rmid_ptrs[r] = entry;
>  
>  		list_add_tail(&entry->list, &cqm_rmid_free_lru);
> @@ -568,7 +570,8 @@ static bool intel_cqm_rmid_stabilize(unsigned int *available)
>  	/*
>  	 * Test whether an RMID is free for each package.
>  	 */
> -	on_each_cpu_mask(&cqm_cpumask, intel_cqm_stable, NULL, true);
> +	if (entry->config)
> +		on_each_cpu_mask(&cqm_cpumask, intel_cqm_stable, NULL, true);
>  

'entry' is a loop variable and so you're just checking the ->config
value of the last RMID on the limbo list. If you've got a mixture of
MBM and CMT RMIDs this might do the wrong thing.

You can only skip the stabilization if there are no CMT RMIDs on the
limbo lru at all.

Also, you need more changes to this function because you're still doing the
minimum queue time delay, even for MBM events, e.g.

	*available = 0;
	list_for_each_entry(entry, &cqm_rmid_limbo_lru, list) {
		unsigned long min_queue_time;
		unsigned long now = jiffies;

		/*
		 * We hold RMIDs placed into limbo for a minimum queue
		 * time. Before the minimum queue time has elapsed we do
		 * not recycle RMIDs.
		 *
		 * The reasoning is that until a sufficient time has
		 * passed since we stopped using an RMID, any RMID
		 * placed onto the limbo list will likely still have
		 * data tagged in the cache, which means we'll probably
		 * fail to recycle it anyway.
		 *
		 * We can save ourselves an expensive IPI by skipping
		 * any RMIDs that have not been queued for the minimum
		 * time.
		 */
		min_queue_time = entry->queue_time +
			msecs_to_jiffies(__rmid_queue_time_ms);

		if (time_after(min_queue_time, now))
			break;

		entry->state = RMID_AVAILABLE;
		(*available)++;
	}


You can mark the RMID as availble irrespective of the time it has been
sat on the limbo list if that RMID has only been used for MBM events.

>  		/*
> @@ -1003,6 +1006,12 @@ static void intel_cqm_event_start(struct perf_event *event, int mode)
>  	}
>  
>  	state->rmid = rmid;
> +	if (event->attr.config & QOS_L3_OCCUP_EVENT_ID) {
> +		struct cqm_rmid_entry *entry;
> +
> +		entry = __rmid_entry(rmid);
> +		entry->config = true;
> +	}
>  	wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);
>  }
  
Because RMIDs can be rotated between events, we need to maintain this
metadata in other locations, not jut intel_cqm_event_start().

Take a look at intel_cqm_xchg_rmid() and intel_cqm_setup_event().
Anywhere we assign an RMID to an event we potentially need to update
whether or not the RMID will need recycling.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/