Re: [PATCH 5/7] thermal: intel: hfi: Enable notification interrupt

On Mon, Nov 08, 2021 at 10:07:40AM +0100, Peter Zijlstra wrote:

On Fri, Nov 05, 2021 at 06:33:10PM -0700, Ricardo Neri wrote:

quoted

@@ -72,6 +78,9 @@ struct hfi_instance {
 	u16			die_id;
 	struct cpumask		*cpus;
 	void			*hw_table;
+	struct delayed_work	update_work;
+	raw_spinlock_t		event_lock;

  +	raw_spinlock_t		interrupt_lock;

Thank you very much for your feedback Peter!

I would like to confirm that I understand your feedback correctly: you are
suggesting to use to spinlocks...

quoted

+	u64			timestamp;
 	bool			initialized;
 };
 

@@ -114,6 +123,75 @@ static struct hfi_instance *hfi_instances;
 static struct hfi_features hfi_features;
 static DEFINE_MUTEX(hfi_lock);
 
+#define HFI_UPDATE_INTERVAL	HZ
+
+static void hfi_update_work_fn(struct work_struct *work)
+{
+	struct hfi_instance *hfi_instance;
+
+	hfi_instance = container_of(to_delayed_work(work), struct hfi_instance,
+				    update_work);
+	if (!hfi_instance)
+		return;
+
+	/* TODO: Consume update here. */

	// this here uses ->event_lock to serialize against the
	// interrupt below changing the data...

Anyone reading the HFI table would need to take ->event_lock.

quoted

+}
+
+void intel_hfi_process_event(__u64 pkg_therm_status_msr_val)
+{
+	struct hfi_instance *hfi_instance;
+	int cpu = smp_processor_id();
+	struct hfi_cpu_info *info;
+	unsigned long flags;
+	u64 timestamp;
+
+	if (!pkg_therm_status_msr_val)
+		return;
+
+	info = &per_cpu(hfi_cpu_info, cpu);
+	if (!info)
+		return;
+
+	/*
+	 * It is possible that we get an HFI thermal interrupt on this CPU
+	 * before its HFI instance is initialized. This is not a problem. The
+	 * CPU that enabled the interrupt for this package will also get the
+	 * interrupt and is fully initialized.
+	 */
+	hfi_instance = info->hfi_instance;
+	if (!hfi_instance)
+		return;
+

	/*
	 * If someone is already handling the interrupt, we shouldn't be
	 * burning time waiting for them to then do more nothing.
	 */
	if (!raw_spin_trylock(&hfi_instance->interrupt_lock))
		return;

quoted

+	raw_spin_lock_irqsave(&hfi_instance->event_lock, flags);

The CPU who acquired ->interrupt_lock successfully now will acquire
->event_lock to serialize writes and reads to the HFI table.

quoted

+
+	/*
+	 * On most systems, all CPUs in the package receive a package-level
+	 * thermal interrupt when there is an HFI update. Since they all are
+	 * dealing with the same update (as indicated by the update timestamp),
+	 * it is sufficient to let a single CPU to acknowledge the update and
+	 * schedule work to process it.
+	 */
+	timestamp = *(u64 *)hfi_instance->hw_table;
+	if (hfi_instance->timestamp >= timestamp)
+		goto unlock_spinlock;

This can go the way of the dodo.

(I guess I can still check the timestamp in case buggy firmware triggers
updates with the same timestamp, right?)

quoted

+
+	hfi_instance->timestamp = timestamp;
+
+	memcpy(hfi_instance->table_base, hfi_instance->hw_table,
+	       hfi_features.nr_table_pages << PAGE_SHIFT);
+	/*
+	 * Let hardware and other CPUs know that we are done reading the HFI
+	 * table and it is free to update it again.
+	 */
+	pkg_therm_status_msr_val &= THERM_STATUS_CLEAR_PKG_MASK &
+				    ~PACKAGE_THERM_STATUS_HFI_UPDATED;
+	wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, pkg_therm_status_msr_val);
+	schedule_delayed_work(&hfi_instance->update_work, HFI_UPDATE_INTERVAL);
+
+unlock_spinlock:
+	raw_spin_unlock_irqrestore(&hfi_instance->event_lock, flags);

	raw_spin_unlock(&hfi_instance->interrupt_lock);

... and here we release both locks.

Thanks and BR,
Ricardo