linux-kernel - Re: [GIT PULL] First batch of KVM changes for 4.1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Apr 2015 14:46:57 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	gleb@...nel.org, kvm@...r.kernel.org,
	Ralf Baechle <ralf@...ux-mips.org>, mtosatti@...hat.com,
	luto@...nel.org
Subject: Re: [GIT PULL] First batch of KVM changes for 4.1



On 17/04/2015 12:55, Peter Zijlstra wrote:
> Also, it looks like you already do exactly this for other things, look
> at:
> 
> 	kvm_sched_in()
> 	  kvm_arch_vcpu_load()
> 	    if (unlikely(vcpu->cpu != cpu) ... )
> 
> So no, I don't believe for one second you need this.

You're missing that this snippet is running in the host, while this 
patch is concerned with the guest (paravirt).

This notifier runs for _all_ tasks, not just for the KVM threads.  In 
fact there will most likely be no KVM in the guest.

There is no vcpu->cpu where this notifier is run.

And frankly, I think the static key is snake oil.  The cost of task 
migration in terms of cache misses and TLB misses is in no way 
comparable to the cost of filling in a structure on the stack, 
dereferencing the head of the notifiers list and seeing that it's NULL.

If this was a real problem, it would be better solved by adding 
inlining in kernel/notifier.c:

diff --git a/kernel/notifier.c b/kernel/notifier.c
index ae9fc7cc360e..9c0cd0f739e0 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -59,28 +59,14 @@ static int notifier_chain_unregister(struct notifier_block **nl,
 	return -ENOENT;
 }
 
-/**
- * notifier_call_chain - Informs the registered notifiers about an event.
- *	@nl:		Pointer to head of the blocking notifier chain
- *	@val:		Value passed unmodified to notifier function
- *	@v:		Pointer passed unmodified to notifier function
- *	@nr_to_call:	Number of notifier functions to be called. Don't care
- *			value of this parameter is -1.
- *	@nr_calls:	Records the number of notifications sent. Don't care
- *			value of this field is NULL.
- *	@returns:	notifier_call_chain returns the value returned by the
- *			last notifier function called.
- */
-static int notifier_call_chain(struct notifier_block **nl,
-			       unsigned long val, void *v,
-			       int nr_to_call, int *nr_calls)
+static int __notifier_call_chain(struct notifier_block *nb,
+			         unsigned long val, void *v,
+			         int nr_to_call, int *nr_calls)
 {
 	int ret = NOTIFY_DONE;
-	struct notifier_block *nb, *next_nb;
-
-	nb = rcu_dereference_raw(*nl);
+	struct notifier_block *next_nb;
 
-	while (nb && nr_to_call) {
+	do {
 		next_nb = rcu_dereference_raw(nb->next);
 
 #ifdef CONFIG_DEBUG_NOTIFIERS
@@ -94,14 +80,38 @@ static int notifier_call_chain(struct notifier_block **nl,
 
 		if (nr_calls)
 			(*nr_calls)++;
-
-		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
-			break;
-		nb = next_nb;
-		nr_to_call--;
-	}
+	} while (!(ret & NOTIFY_STOP_MASK) &&
+		 (nb = next_nb) != NULL &&
+		 --nr_to_call);
 	return ret;
 }
+NOKPROBE_SYMBOL(__notifier_call_chain);
+
+/**
+ * notifier_call_chain - Informs the registered notifiers about an event.
+ *	@nl:		Pointer to head of the blocking notifier chain
+ *	@val:		Value passed unmodified to notifier function
+ *	@v:		Pointer passed unmodified to notifier function
+ *	@nr_to_call:	Number of notifier functions to be called. Don't care
+ *			value of this parameter is -1.
+ *	@nr_calls:	Records the number of notifications sent. Don't care
+ *			value of this field is NULL.
+ *	@returns:	notifier_call_chain returns the value returned by the
+ *			last notifier function called.
+ */
+static __always_inline int notifier_call_chain(struct notifier_block **nl,
+			      		       unsigned long val, void *v,
+					       int nr_to_call, int *nr_calls)
+{
+	struct notifier_block *nb = rcu_dereference_raw(*nl);
+	if (unlikely(nr_to_call == 0))
+		return NOTIFY_DONE;
+
+	if (!nb)
+		return NOTIFY_DONE;
+
+	return __notifier_call_chain(nb, val, v, nr_to_call, nr_calls);
+}
 NOKPROBE_SYMBOL(notifier_call_chain);
 
 /*
@@ -190,7 +199,12 @@ NOKPROBE_SYMBOL(__atomic_notifier_call_chain);
 int atomic_notifier_call_chain(struct atomic_notifier_head *nh,
 			       unsigned long val, void *v)
 {
-	return __atomic_notifier_call_chain(nh, val, v, -1, NULL);
+	int ret;
+
+	rcu_read_lock();
+	ret = notifier_call_chain(&nh->head, val, v, -1, NULL);
+	rcu_read_unlock();
+	return ret;
 }
 EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
 NOKPROBE_SYMBOL(atomic_notifier_call_chain);


Also, move enough stuff to a header so that the fast path is inlined to
a single pointer derefrence.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/