* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Srivatsa S. Bhat @ 2013-02-10 19:41 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130209000717.GP2666@linux.vnet.ibm.com>
On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
>> Once stop_machine() is gone from the CPU offline path, we won't be able to
>> depend on preempt_disable() to prevent CPUs from going offline from under us.
>>
>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
>> while invoking from atomic context.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Would it make sense for get_online_cpus_atomic() to return the current
> CPU number?
Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
return anything (for other reasons, of course..)
Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
And also I don't think it'll make a huge improvement in the callers.. (We might
be better off avoiding an smp_processor_id() if possible, since this function could
be called in very hot paths).. So I don't see a strong case for returning the
CPU number. But let me know if you think it'll still be worth it for some reason...
> Looks good otherwise.
>
Thank you very much for the detailed review, Paul!
Regards,
Srivatsa S. Bhat
>
>> ---
>>
>> kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index 29dd40a..f421bcc 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -310,7 +310,8 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> * prevent preemption and reschedule on another processor,
>> * as well as CPU removal
>> */
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> + this_cpu = smp_processor_id();
>>
>> /*
>> * Can deadlock when called with interrupts disabled.
>> @@ -342,7 +343,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> }
>> }
>>
>> - put_cpu();
>> + put_online_cpus_atomic();
>>
>> return err;
>> }
>> @@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,
>> const struct cpumask *nodemask;
>> int ret;
>>
>> + get_online_cpus_atomic();
>> /* Try for same CPU (cheapest) */
>> - cpu = get_cpu();
>> + cpu = smp_processor_id();
>> +
>> if (cpumask_test_cpu(cpu, mask))
>> goto call;
>>
>> @@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,
>> cpu = cpumask_any_and(mask, cpu_online_mask);
>> call:
>> ret = smp_call_function_single(cpu, func, info, wait);
>> - put_cpu();
>> + put_online_cpus_atomic();
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(smp_call_function_any);
>> @@ -409,25 +412,28 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
>> unsigned int this_cpu;
>> unsigned long flags;
>>
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> +
>> + this_cpu = smp_processor_id();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> * send smp call function interrupt to this cpu and as such deadlocks
>> * can't happen.
>> */
>> - WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
>> + WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
>> && !oops_in_progress);
>>
>> if (cpu == this_cpu) {
>> local_irq_save(flags);
>> data->func(data->info);
>> local_irq_restore(flags);
>> - } else {
>> + } else if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
>> csd_lock(data);
>> generic_exec_single(cpu, data, wait);
>> }
>> - put_cpu();
>> + put_online_cpus_atomic();
>> }
>>
>> /**
>> @@ -451,6 +457,8 @@ void smp_call_function_many(const struct cpumask *mask,
>> unsigned long flags;
>> int refs, cpu, next_cpu, this_cpu = smp_processor_id();
>>
>> + get_online_cpus_atomic();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> @@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,
>>
>> /* No online cpus? We're done. */
>> if (cpu >= nr_cpu_ids)
>> - return;
>> + goto out_unlock;
>>
>> /* Do we have another CPU which isn't us? */
>> next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
>> if (next_cpu == this_cpu)
>> - next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
>> + next_cpu = cpumask_next_and(next_cpu, mask,
>> + cpu_online_mask);
>>
>> /* Fastpath: do that cpu by itself. */
>> if (next_cpu >= nr_cpu_ids) {
>> smp_call_function_single(cpu, func, info, wait);
>> - return;
>> + goto out_unlock;
>> }
>>
>> data = &__get_cpu_var(cfd_data);
>> @@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Some callers race with other cpus changing the passed mask */
>> if (unlikely(!refs)) {
>> csd_unlock(&data->csd);
>> - return;
>> + goto out_unlock;
>> }
>>
>> raw_spin_lock_irqsave(&call_function.lock, flags);
>> @@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Optionally wait for the CPUs to complete */
>> if (wait)
>> csd_lock_wait(&data->csd);
>> +
>> +out_unlock:
>> + put_online_cpus_atomic();
>> }
>> EXPORT_SYMBOL(smp_call_function_many);
>>
>> @@ -574,9 +586,9 @@ EXPORT_SYMBOL(smp_call_function_many);
>> */
>> int smp_call_function(smp_call_func_t func, void *info, int wait)
>> {
>> - preempt_disable();
>> + get_online_cpus_atomic();
>> smp_call_function_many(cpu_online_mask, func, info, wait);
>> - preempt_enable();
>> + put_online_cpus_atomic();
>>
>> return 0;
>> }
>>
^ permalink raw reply
* Re: [PATCH v5 14/45] rcu, CPU hotplug: Fix comment referring to stop_machine()
From: Srivatsa S. Bhat @ 2013-02-10 19:43 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130209001439.GQ2666@linux.vnet.ibm.com>
On 02/09/2013 05:44 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:06:34PM +0530, Srivatsa S. Bhat wrote:
>> Don't refer to stop_machine() in the CPU hotplug path, since we are going
>> to get rid of it. Also, move the comment referring to callback adoption
>> to the CPU_DEAD case, because that's where it happens now.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Ouch! That comment is indeed obsolete and must die.
>
> I queued this to -rcu with your Signed-off-by. However, I omitted
> the added comment, as it is imcomplete -- it is easy to look at
> rcu_cleanup_dead_cpu() to see what it does.
>
Cool! I'll drop it from my patch series then. Thank you!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 44/45] CPU hotplug, stop_machine: Decouple CPU hotplug from stop_machine() in Kconfig
From: Srivatsa S. Bhat @ 2013-02-10 19:45 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130209001554.GR2666@linux.vnet.ibm.com>
On 02/09/2013 05:45 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:15:22PM +0530, Srivatsa S. Bhat wrote:
>> ... and also cleanup a comment that refers to CPU hotplug being dependent on
>> stop_machine().
>>
>> Cc: David Howells <dhowells@redhat.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> (Hey, I thought I owed myself an easy one!)
>
Haha ;-)
Regards,
Srivatsa S. Bhat
>> ---
>>
>> include/linux/stop_machine.h | 2 +-
>> init/Kconfig | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
>> index 3b5e910..ce2d3c4 100644
>> --- a/include/linux/stop_machine.h
>> +++ b/include/linux/stop_machine.h
>> @@ -120,7 +120,7 @@ int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
>> * @cpus: the cpus to run the @fn() on (NULL = any online cpu)
>> *
>> * Description: This is a special version of the above, which assumes cpus
>> - * won't come or go while it's being called. Used by hotplug cpu.
>> + * won't come or go while it's being called.
>> */
>> int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index be8b7f5..048a0c5 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1711,7 +1711,7 @@ config INIT_ALL_POSSIBLE
>> config STOP_MACHINE
>> bool
>> default y
>> - depends on (SMP && MODULE_UNLOAD) || HOTPLUG_CPU
>> + depends on (SMP && MODULE_UNLOAD)
>> help
>> Need stop_machine() primitive.
>>
>>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 19:47 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <5117F0C0.2030605@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 12:40:56AM +0530, Srivatsa S. Bhat wrote:
> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
> > On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
> >> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
> >> lock-ordering related problems (unlike per-cpu locks). However, global
> >> rwlocks lead to unnecessary cache-line bouncing even when there are no
> >> writers present, which can slow down the system needlessly.
> >>
> [...]
> >
> > Looks pretty close! Some comments interspersed below. Please either
> > fix the code or my confusion, as the case may be. ;-)
> >
>
> Sure :-)
>
> >> ---
> >>
> >> include/linux/percpu-rwlock.h | 10 +++
> >> lib/percpu-rwlock.c | 128 ++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 136 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/percpu-rwlock.h b/include/linux/percpu-rwlock.h
> >> index 8dec8fe..6819bb8 100644
> >> --- a/include/linux/percpu-rwlock.h
> >> +++ b/include/linux/percpu-rwlock.h
> >> @@ -68,4 +68,14 @@ extern void percpu_free_rwlock(struct percpu_rwlock *);
> >> __percpu_init_rwlock(pcpu_rwlock, #pcpu_rwlock, &rwlock_key); \
> >> })
> >>
> >> +#define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
> >> + (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
> >> +
> >> +#define reader_nested_percpu(pcpu_rwlock) \
> >> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
> >> +
> >> +#define writer_active(pcpu_rwlock) \
> >> + (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
> >> +
> >> #endif
> >> +
> >> diff --git a/lib/percpu-rwlock.c b/lib/percpu-rwlock.c
> >> index 80dad93..992da5c 100644
> >> --- a/lib/percpu-rwlock.c
> >> +++ b/lib/percpu-rwlock.c
> >> @@ -64,21 +64,145 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)
> >>
> >> void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> - read_lock(&pcpu_rwlock->global_rwlock);
> >> + preempt_disable();
> >> +
> >> + /* First and foremost, let the writer know that a reader is active */
> >> + this_cpu_inc(*pcpu_rwlock->reader_refcnt);
> >> +
> >> + /*
> >> + * If we are already using per-cpu refcounts, it is not safe to switch
> >> + * the synchronization scheme. So continue using the refcounts.
> >> + */
> >> + if (reader_nested_percpu(pcpu_rwlock)) {
> >> + goto out;
> >> + } else {
> >> + /*
> >> + * The write to 'reader_refcnt' must be visible before we
> >> + * read 'writer_signal'.
> >> + */
> >> + smp_mb(); /* Paired with smp_rmb() in sync_reader() */
> >> +
> >> + if (likely(!writer_active(pcpu_rwlock))) {
> >> + goto out;
> >> + } else {
> >> + /* Writer is active, so switch to global rwlock. */
> >> + read_lock(&pcpu_rwlock->global_rwlock);
> >> +
> >> + /*
> >> + * We might have raced with a writer going inactive
> >> + * before we took the read-lock. So re-evaluate whether
> >> + * we still need to hold the rwlock or if we can switch
> >> + * back to per-cpu refcounts. (This also helps avoid
> >> + * heterogeneous nesting of readers).
> >> + */
> >> + if (writer_active(pcpu_rwlock))
> >
> > The above writer_active() can be reordered with the following this_cpu_dec(),
> > strange though it might seem. But this is OK because holding the rwlock
> > is conservative. But might be worth a comment.
> >
>
> Ok..
>
> >> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
> >> + else
> >
> > In contrast, no reordering can happen here because read_unlock() is
> > required to keep the critical section underneath the lock.
> >
>
> Ok..
>
> >> + read_unlock(&pcpu_rwlock->global_rwlock);
> >> + }
> >> + }
> >> +
> >> +out:
> >> + /* Prevent reordering of any subsequent reads */
> >> + smp_rmb();
> >
> > This should be smp_mb(). "Readers" really can do writes. Hence the
> > name lglock -- "local/global" rather than "reader/writer".
> >
>
> Ok!
>
> [ We were trying to avoid full memory barriers in get/put_online_cpus_atomic()
> in the fastpath, as far as possible... Now it feels like all that discussion
> was for nothing :-( ]
>
> >> }
> >>
> >> void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> - read_unlock(&pcpu_rwlock->global_rwlock);
> >
> > We need an smp_mb() here to keep the critical section ordered before the
> > this_cpu_dec() below. Otherwise, if a writer shows up just after we
> > exit the fastpath, that writer is not guaranteed to see the effects of
> > our critical section. Equivalently, the prior read-side critical section
> > just might see some of the writer's updates, which could be a bit of
> > a surprise to the reader.
> >
>
> Ok, will add it.
>
> >> + /*
> >> + * We never allow heterogeneous nesting of readers. So it is trivial
> >> + * to find out the kind of reader we are, and undo the operation
> >> + * done by our corresponding percpu_read_lock().
> >> + */
> >> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
> >> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
> >> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
> >
> > Given an smp_mb() above, I don't understand the need for this smp_wmb().
> > Isn't the idea that if the writer sees ->reader_refcnt decremented to
> > zero, it also needs to see the effects of the corresponding reader's
> > critical section?
> >
>
> Not sure what you meant, but my idea here was that the writer should see
> the reader_refcnt falling to zero as soon as possible, to avoid keeping the
> writer waiting in a tight loop for longer than necessary.
> I might have been a little over-zealous to use lighter memory barriers though,
> (given our lengthy discussions in the previous versions to reduce the memory
> barrier overheads), so the smp_wmb() used above might be wrong.
>
> So, are you saying that the smp_mb() you indicated above would be enough
> to make the writer observe the 1->0 transition of reader_refcnt immediately?
>
> > Or am I missing something subtle here? In any case, if this smp_wmb()
> > really is needed, there should be some subsequent write that the writer
> > might observe. From what I can see, there is no subsequent write from
> > this reader that the writer cares about.
>
> I thought the smp_wmb() here and the smp_rmb() at the writer would ensure
> immediate reflection of the reader state at the writer side... Please correct
> me if my understanding is incorrect.
Ah, but memory barriers are not so much about making data move faster
through the machine, but more about making sure that ordering constraints
are met. After all, memory barriers cannot make electrons flow faster
through silicon. You should therefore use memory barriers only to
constrain ordering, not to try to expedite electrons.
> >> + } else {
> >> + read_unlock(&pcpu_rwlock->global_rwlock);
> >> + }
> >> +
> >> + preempt_enable();
> >> +}
> >> +
> >> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
> >> +}
> >> +
> >> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
> >> +}
> >> +
> >> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + for_each_online_cpu(cpu)
> >> + raise_writer_signal(pcpu_rwlock, cpu);
> >> +
> >> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> >> +}
> >> +
> >> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >
> > Why do we drop ourselves twice? More to the point, why is it important to
> > drop ourselves first?
>
> I don't see where we are dropping ourselves twice. Note that we are no longer
> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
> to manually drop ourselves. It doesn't matter whether we drop ourselves first
> or later.
Good point, apologies for my confusion! Still worth a commment, though.
> >> +
> >> + for_each_online_cpu(cpu)
> >> + drop_writer_signal(pcpu_rwlock, cpu);
> >> +
> >> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> >> +}
> >> +
> >> +/*
> >> + * Wait for the reader to see the writer's signal and switch from percpu
> >> + * refcounts to global rwlock.
> >> + *
> >> + * If the reader is still using percpu refcounts, wait for him to switch.
> >> + * Else, we can safely go ahead, because either the reader has already
> >> + * switched over, or the next reader that comes along on that CPU will
> >> + * notice the writer's signal and will switch over to the rwlock.
> >> + */
> >> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >> + unsigned int cpu)
> >> +{
> >> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >
> > As I understand it, the purpose of this memory barrier is to ensure
> > that the stores in drop_writer_signal() happen before the reads from
> > ->reader_refcnt in reader_uses_percpu_refcnt(),
>
> No, that was not what I intended. announce_writer_inactive() already does
> a full smp_mb() after calling drop_writer_signal().
>
> I put the smp_rmb() here and the smp_wmb() at the reader side (after updates
> to the ->reader_refcnt) to reflect the state change of ->reader_refcnt
> immediately at the writer, so that the writer doesn't have to keep spinning
> unnecessarily still referring to the old (non-zero) value of ->reader_refcnt.
> Or perhaps I am confused about how to use memory barriers properly.. :-(
Sadly, no, memory barriers don't make electrons move faster. So you
should only need the one -- the additional memory barriers are just
slowing things down.
> > thus preventing the
> > race between a new reader attempting to use the fastpath and this writer
> > acquiring the lock. Unless I am confused, this must be smp_mb() rather
> > than smp_rmb().
> >
> > Also, why not just have a single smp_mb() at the beginning of
> > sync_all_readers() instead of executing one barrier per CPU?
>
> Well, since my intention was to help the writer see the update (->reader_refcnt
> dropping to zero) ASAP, I kept the multiple smp_rmb()s.
At least you were consistent. ;-)
> >> +
> >> + while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
> >> + cpu_relax();
> >> +}
> >> +
> >> +static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + for_each_online_cpu(cpu)
> >> + sync_reader(pcpu_rwlock, cpu);
> >> }
> >>
> >> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> + /*
> >> + * Tell all readers that a writer is becoming active, so that they
> >> + * start switching over to the global rwlock.
> >> + */
> >> + announce_writer_active(pcpu_rwlock);
> >> + sync_all_readers(pcpu_rwlock);
> >> write_lock(&pcpu_rwlock->global_rwlock);
> >> }
> >>
> >> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
> >> {
> >> + /*
> >> + * Inform all readers that we are done, so that they can switch back
> >> + * to their per-cpu refcounts. (We don't need to wait for them to
> >> + * see it).
> >> + */
> >> + announce_writer_inactive(pcpu_rwlock);
> >> write_unlock(&pcpu_rwlock->global_rwlock);
> >> }
> >>
> >>
>
> Thanks a lot for your detailed review and comments! :-)
It will be good to get this in!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Oleg Nesterov @ 2013-02-10 19:50 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, Paul E. McKenney, nikunj, linux-pm, rusty,
rostedt, rjw, namhyung, tglx, linux-arm-kernel, netdev,
linux-kernel, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <5117F403.1050300@linux.vnet.ibm.com>
On 02/11, Srivatsa S. Bhat wrote:
>
> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
> >>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >>> +{
> >>> + unsigned int cpu;
> >>> +
> >>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >>
> >> Why do we drop ourselves twice? More to the point, why is it important to
> >> drop ourselves first?
> >
> > And don't we need mb() _before_ we clear ->writer_signal ?
> >
>
> Oh, right! Or, how about moving announce_writer_inactive() to _after_
> write_unlock()?
Not sure this will help... but, either way it seems we have another
problem...
percpu_rwlock tries to be "generic". This means we should "ignore" its
usage in hotplug, and _write_lock should not race with _write_unlock.
IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
that this can't race with another write which wants to set this flag.
Perhaps it should be counter as well, and it should be protected by
the same ->global_rwlock, but _write_lock() should drop it before
sync_all_readers() and then take it again?
> >>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >>> + unsigned int cpu)
> >>> +{
> >>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >>
> >> As I understand it, the purpose of this memory barrier is to ensure
> >> that the stores in drop_writer_signal() happen before the reads from
> >> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> >> race between a new reader attempting to use the fastpath and this writer
> >> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> >> than smp_rmb().
> >
> > And note that before sync_reader() we call announce_writer_active() which
> > already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> > unneeded.
> >
>
> My intention was to help the writer see the ->reader_refcnt drop to zero
> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
Hmm, interesting... Not sure, but can't really comment. However I can
answer your next question:
> Please correct me if my understanding of memory barriers is wrong here..
Who? Me??? No we have paulmck for that ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 19:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt, rjw,
namhyung, tglx, linux-arm-kernel, netdev, linux-kernel, sbw,
Srivatsa S. Bhat, tj, akpm, linuxppc-dev
In-Reply-To: <20130210180607.GA1375@redhat.com>
On Sun, Feb 10, 2013 at 07:06:07PM +0100, Oleg Nesterov wrote:
> On 02/08, Paul E. McKenney wrote:
[ . . . ]
> > > +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> > > + unsigned int cpu)
> > > +{
> > > + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >
> > As I understand it, the purpose of this memory barrier is to ensure
> > that the stores in drop_writer_signal() happen before the reads from
> > ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> > race between a new reader attempting to use the fastpath and this writer
> > acquiring the lock. Unless I am confused, this must be smp_mb() rather
> > than smp_rmb().
>
> And note that before sync_reader() we call announce_writer_active() which
> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> unneeded.
>
> But, at the same time, could you confirm that we do not need another mb()
> after sync_all_readers() in percpu_write_lock() ? I mean, without mb(),
> can't this reader_uses_percpu_refcnt() LOAD leak into the critical section
> protected by ->global_rwlock? Then this LOAD can be re-ordered with other
> memory operations done by the writer.
As soon as I get the rest of the way through Thomas's patchset. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Paul E. McKenney @ 2013-02-10 19:56 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <5117F7E9.7070906@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 01:11:29AM +0530, Srivatsa S. Bhat wrote:
> On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
> > On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
> >> Once stop_machine() is gone from the CPU offline path, we won't be able to
> >> depend on preempt_disable() to prevent CPUs from going offline from under us.
> >>
> >> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
> >> while invoking from atomic context.
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >
> > Would it make sense for get_online_cpus_atomic() to return the current
> > CPU number?
>
> Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
> return anything (for other reasons, of course..)
>
> Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
> But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
>
> And also I don't think it'll make a huge improvement in the callers.. (We might
> be better off avoiding an smp_processor_id() if possible, since this function could
> be called in very hot paths).. So I don't see a strong case for returning the
> CPU number. But let me know if you think it'll still be worth it for some reason...
I just noted a lot of two-line code sequences in your patch that would be
one line if the CPU number was returned. But I don't feel strongly about
it, so if people are OK with the current version, no problem.
Thanx, Paul
> > Looks good otherwise.
> >
>
> Thank you very much for the detailed review, Paul!
>
> Regards,
> Srivatsa S. Bhat
>
> >
> >> ---
> >>
> >> kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
> >> 1 file changed, 26 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/kernel/smp.c b/kernel/smp.c
> >> index 29dd40a..f421bcc 100644
> >> --- a/kernel/smp.c
> >> +++ b/kernel/smp.c
> >> @@ -310,7 +310,8 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
> >> * prevent preemption and reschedule on another processor,
> >> * as well as CPU removal
> >> */
> >> - this_cpu = get_cpu();
> >> + get_online_cpus_atomic();
> >> + this_cpu = smp_processor_id();
> >>
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> @@ -342,7 +343,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
> >> }
> >> }
> >>
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >>
> >> return err;
> >> }
> >> @@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,
> >> const struct cpumask *nodemask;
> >> int ret;
> >>
> >> + get_online_cpus_atomic();
> >> /* Try for same CPU (cheapest) */
> >> - cpu = get_cpu();
> >> + cpu = smp_processor_id();
> >> +
> >> if (cpumask_test_cpu(cpu, mask))
> >> goto call;
> >>
> >> @@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,
> >> cpu = cpumask_any_and(mask, cpu_online_mask);
> >> call:
> >> ret = smp_call_function_single(cpu, func, info, wait);
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >> return ret;
> >> }
> >> EXPORT_SYMBOL_GPL(smp_call_function_any);
> >> @@ -409,25 +412,28 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
> >> unsigned int this_cpu;
> >> unsigned long flags;
> >>
> >> - this_cpu = get_cpu();
> >> + get_online_cpus_atomic();
> >> +
> >> + this_cpu = smp_processor_id();
> >> +
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> * We allow cpu's that are not yet online though, as no one else can
> >> * send smp call function interrupt to this cpu and as such deadlocks
> >> * can't happen.
> >> */
> >> - WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
> >> + WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
> >> && !oops_in_progress);
> >>
> >> if (cpu == this_cpu) {
> >> local_irq_save(flags);
> >> data->func(data->info);
> >> local_irq_restore(flags);
> >> - } else {
> >> + } else if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
> >> csd_lock(data);
> >> generic_exec_single(cpu, data, wait);
> >> }
> >> - put_cpu();
> >> + put_online_cpus_atomic();
> >> }
> >>
> >> /**
> >> @@ -451,6 +457,8 @@ void smp_call_function_many(const struct cpumask *mask,
> >> unsigned long flags;
> >> int refs, cpu, next_cpu, this_cpu = smp_processor_id();
> >>
> >> + get_online_cpus_atomic();
> >> +
> >> /*
> >> * Can deadlock when called with interrupts disabled.
> >> * We allow cpu's that are not yet online though, as no one else can
> >> @@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,
> >>
> >> /* No online cpus? We're done. */
> >> if (cpu >= nr_cpu_ids)
> >> - return;
> >> + goto out_unlock;
> >>
> >> /* Do we have another CPU which isn't us? */
> >> next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> >> if (next_cpu == this_cpu)
> >> - next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
> >> + next_cpu = cpumask_next_and(next_cpu, mask,
> >> + cpu_online_mask);
> >>
> >> /* Fastpath: do that cpu by itself. */
> >> if (next_cpu >= nr_cpu_ids) {
> >> smp_call_function_single(cpu, func, info, wait);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> data = &__get_cpu_var(cfd_data);
> >> @@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,
> >> /* Some callers race with other cpus changing the passed mask */
> >> if (unlikely(!refs)) {
> >> csd_unlock(&data->csd);
> >> - return;
> >> + goto out_unlock;
> >> }
> >>
> >> raw_spin_lock_irqsave(&call_function.lock, flags);
> >> @@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,
> >> /* Optionally wait for the CPUs to complete */
> >> if (wait)
> >> csd_lock_wait(&data->csd);
> >> +
> >> +out_unlock:
> >> + put_online_cpus_atomic();
> >> }
> >> EXPORT_SYMBOL(smp_call_function_many);
> >>
> >> @@ -574,9 +586,9 @@ EXPORT_SYMBOL(smp_call_function_many);
> >> */
> >> int smp_call_function(smp_call_func_t func, void *info, int wait)
> >> {
> >> - preempt_disable();
> >> + get_online_cpus_atomic();
> >> smp_call_function_many(cpu_online_mask, func, info, wait);
> >> - preempt_enable();
> >> + put_online_cpus_atomic();
> >>
> >> return 0;
> >> }
> >>
>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 19:57 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130210194759.GJ2666@linux.vnet.ibm.com>
On 02/11/2013 01:17 AM, Paul E. McKenney wrote:
> On Mon, Feb 11, 2013 at 12:40:56AM +0530, Srivatsa S. Bhat wrote:
>> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
>>> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
>>>> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
>>>> lock-ordering related problems (unlike per-cpu locks). However, global
>>>> rwlocks lead to unnecessary cache-line bouncing even when there are no
>>>> writers present, which can slow down the system needlessly.
>>>>
>> [...]
>>>> + /*
>>>> + * We never allow heterogeneous nesting of readers. So it is trivial
>>>> + * to find out the kind of reader we are, and undo the operation
>>>> + * done by our corresponding percpu_read_lock().
>>>> + */
>>>> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
>>>> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
>>>> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
>>>
>>> Given an smp_mb() above, I don't understand the need for this smp_wmb().
>>> Isn't the idea that if the writer sees ->reader_refcnt decremented to
>>> zero, it also needs to see the effects of the corresponding reader's
>>> critical section?
>>>
>>
>> Not sure what you meant, but my idea here was that the writer should see
>> the reader_refcnt falling to zero as soon as possible, to avoid keeping the
>> writer waiting in a tight loop for longer than necessary.
>> I might have been a little over-zealous to use lighter memory barriers though,
>> (given our lengthy discussions in the previous versions to reduce the memory
>> barrier overheads), so the smp_wmb() used above might be wrong.
>>
>> So, are you saying that the smp_mb() you indicated above would be enough
>> to make the writer observe the 1->0 transition of reader_refcnt immediately?
>>
>>> Or am I missing something subtle here? In any case, if this smp_wmb()
>>> really is needed, there should be some subsequent write that the writer
>>> might observe. From what I can see, there is no subsequent write from
>>> this reader that the writer cares about.
>>
>> I thought the smp_wmb() here and the smp_rmb() at the writer would ensure
>> immediate reflection of the reader state at the writer side... Please correct
>> me if my understanding is incorrect.
>
> Ah, but memory barriers are not so much about making data move faster
> through the machine, but more about making sure that ordering constraints
> are met. After all, memory barriers cannot make electrons flow faster
> through silicon. You should therefore use memory barriers only to
> constrain ordering, not to try to expedite electrons.
>
I guess I must have been confused after looking at that graph which showed
how much time it takes for other CPUs to notice the change in value of a
variable performed in a given CPU.. and must have gotten the (wrong) idea
that memory barriers also help speed that up! Very sorry about that!
>>>> + } else {
>>>> + read_unlock(&pcpu_rwlock->global_rwlock);
>>>> + }
>>>> +
>>>> + preempt_enable();
>>>> +}
>>>> +
>>>> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
>>>> +}
>>>> +
>>>> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
>>>> +}
>>>> +
>>>> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + raise_writer_signal(pcpu_rwlock, cpu);
>>>> +
>>>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>>>> +}
>>>> +
>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>
>>> Why do we drop ourselves twice? More to the point, why is it important to
>>> drop ourselves first?
>>
>> I don't see where we are dropping ourselves twice. Note that we are no longer
>> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
>> to manually drop ourselves. It doesn't matter whether we drop ourselves first
>> or later.
>
> Good point, apologies for my confusion! Still worth a commment, though.
>
Sure, will add it.
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + drop_writer_signal(pcpu_rwlock, cpu);
>>>> +
>>>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>>>> +}
>>>> +
>>>> +/*
>>>> + * Wait for the reader to see the writer's signal and switch from percpu
>>>> + * refcounts to global rwlock.
>>>> + *
>>>> + * If the reader is still using percpu refcounts, wait for him to switch.
>>>> + * Else, we can safely go ahead, because either the reader has already
>>>> + * switched over, or the next reader that comes along on that CPU will
>>>> + * notice the writer's signal and will switch over to the rwlock.
>>>> + */
>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>>>> + unsigned int cpu)
>>>> +{
>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>>>
>>> As I understand it, the purpose of this memory barrier is to ensure
>>> that the stores in drop_writer_signal() happen before the reads from
>>> ->reader_refcnt in reader_uses_percpu_refcnt(),
>>
>> No, that was not what I intended. announce_writer_inactive() already does
>> a full smp_mb() after calling drop_writer_signal().
>>
>> I put the smp_rmb() here and the smp_wmb() at the reader side (after updates
>> to the ->reader_refcnt) to reflect the state change of ->reader_refcnt
>> immediately at the writer, so that the writer doesn't have to keep spinning
>> unnecessarily still referring to the old (non-zero) value of ->reader_refcnt.
>> Or perhaps I am confused about how to use memory barriers properly.. :-(
>
> Sadly, no, memory barriers don't make electrons move faster. So you
> should only need the one -- the additional memory barriers are just
> slowing things down.
>
Ok..
>>> thus preventing the
>>> race between a new reader attempting to use the fastpath and this writer
>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
>>> than smp_rmb().
>>>
>>> Also, why not just have a single smp_mb() at the beginning of
>>> sync_all_readers() instead of executing one barrier per CPU?
>>
>> Well, since my intention was to help the writer see the update (->reader_refcnt
>> dropping to zero) ASAP, I kept the multiple smp_rmb()s.
>
> At least you were consistent. ;-)
>
Haha, that's an optimistic way of looking at it, but its no good if I was
consistently _wrong_! ;-)
>>>> +
>>>> + while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
>>>> + cpu_relax();
>>>> +}
>>>> +
>>>> +static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + for_each_online_cpu(cpu)
>>>> + sync_reader(pcpu_rwlock, cpu);
>>>> }
>>>>
>>>> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
>>>> {
>>>> + /*
>>>> + * Tell all readers that a writer is becoming active, so that they
>>>> + * start switching over to the global rwlock.
>>>> + */
>>>> + announce_writer_active(pcpu_rwlock);
>>>> + sync_all_readers(pcpu_rwlock);
>>>> write_lock(&pcpu_rwlock->global_rwlock);
>>>> }
>>>>
>>>> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
>>>> {
>>>> + /*
>>>> + * Inform all readers that we are done, so that they can switch back
>>>> + * to their per-cpu refcounts. (We don't need to wait for them to
>>>> + * see it).
>>>> + */
>>>> + announce_writer_inactive(pcpu_rwlock);
>>>> write_unlock(&pcpu_rwlock->global_rwlock);
>>>> }
>>>>
>>>>
>>
>> Thanks a lot for your detailed review and comments! :-)
>
> It will be good to get this in!
>
Thank you :-) I'll try to address the review comments and respin the
patchset soon.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Srivatsa S. Bhat @ 2013-02-10 19:59 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, oleg, sbw, tj,
akpm, linuxppc-dev
In-Reply-To: <20130210195639.GL2666@linux.vnet.ibm.com>
On 02/11/2013 01:26 AM, Paul E. McKenney wrote:
> On Mon, Feb 11, 2013 at 01:11:29AM +0530, Srivatsa S. Bhat wrote:
>> On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
>>> On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
>>>> Once stop_machine() is gone from the CPU offline path, we won't be able to
>>>> depend on preempt_disable() to prevent CPUs from going offline from under us.
>>>>
>>>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
>>>> while invoking from atomic context.
>>>>
>>>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>>
>>> Would it make sense for get_online_cpus_atomic() to return the current
>>> CPU number?
>>
>> Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
>> return anything (for other reasons, of course..)
>>
>> Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
>> But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
>>
>> And also I don't think it'll make a huge improvement in the callers.. (We might
>> be better off avoiding an smp_processor_id() if possible, since this function could
>> be called in very hot paths).. So I don't see a strong case for returning the
>> CPU number. But let me know if you think it'll still be worth it for some reason...
>
> I just noted a lot of two-line code sequences in your patch that would be
> one line if the CPU number was returned.
Ah, in that case, I'll reconsider your suggestion while working on the next version.
Thanks!
Regards,
Srivatsa S. Bhat
> But I don't feel strongly about
> it, so if people are OK with the current version, no problem.
>
> Thanx, Paul
>
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 20:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, Paul E. McKenney, nikunj, linux-pm, rusty,
rostedt, rjw, namhyung, tglx, linux-arm-kernel, netdev,
linux-kernel, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <20130210195042.GA6236@redhat.com>
On 02/11/2013 01:20 AM, Oleg Nesterov wrote:
> On 02/11, Srivatsa S. Bhat wrote:
>>
>> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
>>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>>> +{
>>>>> + unsigned int cpu;
>>>>> +
>>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>>
>>>> Why do we drop ourselves twice? More to the point, why is it important to
>>>> drop ourselves first?
>>>
>>> And don't we need mb() _before_ we clear ->writer_signal ?
>>>
>>
>> Oh, right! Or, how about moving announce_writer_inactive() to _after_
>> write_unlock()?
>
> Not sure this will help... but, either way it seems we have another
> problem...
>
> percpu_rwlock tries to be "generic". This means we should "ignore" its
> usage in hotplug, and _write_lock should not race with _write_unlock.
>
Yes, good point!
> IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
> that this can't race with another write which wants to set this flag.
> Perhaps it should be counter as well, and it should be protected by
> the same ->global_rwlock, but _write_lock() should drop it before
> sync_all_readers() and then take it again?
Hmm, or we could just add an extra mb() like you suggested, and keep it
simple...
>
>>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>>>>> + unsigned int cpu)
>>>>> +{
>>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>>>>
>>>> As I understand it, the purpose of this memory barrier is to ensure
>>>> that the stores in drop_writer_signal() happen before the reads from
>>>> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
>>>> race between a new reader attempting to use the fastpath and this writer
>>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
>>>> than smp_rmb().
>>>
>>> And note that before sync_reader() we call announce_writer_active() which
>>> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
>>> unneeded.
>>>
>>
>> My intention was to help the writer see the ->reader_refcnt drop to zero
>> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
>
> Hmm, interesting... Not sure, but can't really comment. However I can
> answer your next question:
>
Paul told me in another mail that I was expecting too much out of memory
barriers, like increasing the speed of electrons and what not ;-)
[ It would have been cool though, if it had such magical powers :P ]
>> Please correct me if my understanding of memory barriers is wrong here..
>
> Who? Me??? No we have paulmck for that ;)
>
Haha ;-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Oleg Nesterov @ 2013-02-10 20:13 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, linux-kernel, sbw,
tj, akpm, linuxppc-dev
In-Reply-To: <5117F0C0.2030605@linux.vnet.ibm.com>
On 02/11, Srivatsa S. Bhat wrote:
>
> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
> >> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >> +{
> >> + unsigned int cpu;
> >> +
> >> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >
> > Why do we drop ourselves twice? More to the point, why is it important to
> > drop ourselves first?
> >
>
> I don't see where we are dropping ourselves twice. Note that we are no longer
> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
> to manually drop ourselves. It doesn't matter whether we drop ourselves first
> or later.
Yes, but this just reflects its usage in cpu-hotplug. cpu goes away under
_write_lock.
Perhaps _write_lock/unlock shoud use for_each_possible_cpu() instead?
Hmm... I think this makes sense anyway. Otherwise, in theory,
percpu_write_lock(random_non_hotplug_lock) can race with cpu_up?
Oleg.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 20:20 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-doc, peterz, fweisbec, mingo, linux-arch, linux,
xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, linux-kernel, sbw,
tj, akpm, linuxppc-dev
In-Reply-To: <20130210201312.GB6236@redhat.com>
On 02/11/2013 01:43 AM, Oleg Nesterov wrote:
> On 02/11, Srivatsa S. Bhat wrote:
>>
>> On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>>> +{
>>>> + unsigned int cpu;
>>>> +
>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>>
>>> Why do we drop ourselves twice? More to the point, why is it important to
>>> drop ourselves first?
>>>
>>
>> I don't see where we are dropping ourselves twice. Note that we are no longer
>> in the cpu_online_mask, so the 'for' loop below won't include us. So we need
>> to manually drop ourselves. It doesn't matter whether we drop ourselves first
>> or later.
>
> Yes, but this just reflects its usage in cpu-hotplug. cpu goes away under
> _write_lock.
>
Ah, right. I guess the code still has remnants from the older version in which
this locking scheme wasn't generic and was tied to cpu-hotplug alone..
> Perhaps _write_lock/unlock shoud use for_each_possible_cpu() instead?
>
Hmm, that wouldn't be too bad.
> Hmm... I think this makes sense anyway. Otherwise, in theory,
> percpu_write_lock(random_non_hotplug_lock) can race with cpu_up?
>
Yeah, makes sense. Will change it to for_each_possible_cpu().
And I had previously fixed such races with lglocks with a complicated scheme (to
avoid the costly for_each_possible loop), which was finally rewritten to use
for_each_possible_cpu() for the sake of simplicity..
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-10 22:13 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, nikunj, linux-pm, rusty, rostedt,
rjw, namhyung, tglx, linux-arm-kernel, netdev, Oleg Nesterov, sbw,
tj, akpm, linuxppc-dev
In-Reply-To: <5117FE74.4020000@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 01:39:24AM +0530, Srivatsa S. Bhat wrote:
> On 02/11/2013 01:20 AM, Oleg Nesterov wrote:
> > On 02/11, Srivatsa S. Bhat wrote:
> >>
> >> On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
> >>>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> >>>>> +{
> >>>>> + unsigned int cpu;
> >>>>> +
> >>>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> >>>>
> >>>> Why do we drop ourselves twice? More to the point, why is it important to
> >>>> drop ourselves first?
> >>>
> >>> And don't we need mb() _before_ we clear ->writer_signal ?
> >>>
> >>
> >> Oh, right! Or, how about moving announce_writer_inactive() to _after_
> >> write_unlock()?
> >
> > Not sure this will help... but, either way it seems we have another
> > problem...
> >
> > percpu_rwlock tries to be "generic". This means we should "ignore" its
> > usage in hotplug, and _write_lock should not race with _write_unlock.
> >
>
> Yes, good point!
>
> > IOW. Suppose that _write_unlock clears ->writer_signal. We need to ensure
> > that this can't race with another write which wants to set this flag.
> > Perhaps it should be counter as well, and it should be protected by
> > the same ->global_rwlock, but _write_lock() should drop it before
> > sync_all_readers() and then take it again?
>
> Hmm, or we could just add an extra mb() like you suggested, and keep it
> simple...
>
> >
> >>>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> >>>>> + unsigned int cpu)
> >>>>> +{
> >>>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> >>>>
> >>>> As I understand it, the purpose of this memory barrier is to ensure
> >>>> that the stores in drop_writer_signal() happen before the reads from
> >>>> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> >>>> race between a new reader attempting to use the fastpath and this writer
> >>>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> >>>> than smp_rmb().
> >>>
> >>> And note that before sync_reader() we call announce_writer_active() which
> >>> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> >>> unneeded.
> >>>
> >>
> >> My intention was to help the writer see the ->reader_refcnt drop to zero
> >> ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
> >
> > Hmm, interesting... Not sure, but can't really comment. However I can
> > answer your next question:
>
> Paul told me in another mail that I was expecting too much out of memory
> barriers, like increasing the speed of electrons and what not ;-)
> [ It would have been cool though, if it had such magical powers :P ]
"But because you have used the special mb_tachyonic instruction, the
speed of light is 600,000 km/s for the next five clock cycles"... ;-)
Thanx, Paul
> >> Please correct me if my understanding of memory barriers is wrong here..
> >
> > Who? Me??? No we have paulmck for that ;)
> >
>
> Haha ;-)
>
> Regards,
> Srivatsa S. Bhat
>
^ permalink raw reply
* Re: PS3 platform is broken on Linux 3.7.0
From: Michael Ellerman @ 2013-02-11 3:39 UTC (permalink / raw)
To: Phileas Fogg; +Cc: linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <1360518697.255951128@f337.mail.ru>
On Sun, Feb 10, 2013 at 09:51:37PM +0400, Phileas Fogg wrote:
>
> >Phileas Fogg < phileas-fogg@mail.ru > writes:
> >
>
> Patch:
>
> --- arch/powerpc/kernel/setup_64.c.old 2013-02-10 19:34:53.787366191 +0100
> +++ arch/powerpc/kernel/setup_64.c 2013-02-10 19:35:38.834035478 +0100
> @@ -186,6 +186,9 @@
> initialise_paca(&boot_paca, 0);
> setup_paca(&boot_paca);
>
> + /* Allow percpu accesses to "work" until we setup percpu data */
> + boot_paca.data_offset = 0;
> +
This is correct.
> /* Initialize lockdep early or else spinlocks will blow */
> lockdep_init();
>
> @@ -208,8 +211,6 @@
>
> /* Fix up paca fields required for the boot cpu */
> get_paca()->cpu_start = 1;
> - /* Allow percpu accesses to "work" until we setup percpu data */
> - get_paca()->data_offset = 0;
But this is not.
As you said, they are different pacas, so we need to make sure both
boot_paca, and "the paca of the boot cpu" are initialised with
data_offset = 0.
I'll send a patch to sort it.
cheers
^ permalink raw reply
* [PATCH] iommu: adding missing kvm_iommu_map_pages/kvm_iommu_unmap_pages
From: Alexey Kardashevskiy @ 2013-02-11 5:09 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, linux-kernel, kvm-ppc, Paul Mackerras,
linuxppc-dev, David Gibson
The IOMMU API implements groups creating/deletion, device binding
and IOMMU map/unmap operations.
The POWERPC implementation uses most of the API except map/unmap
operations which are implemented on POWERPC using hypercalls.
However in order to link a kernel with the CONFIG_IOMMU_API enabled,
the empty kvm_iommu_map_pages/kvm_iommu_unmap_pages have to be
defined, so does the patch.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: David Gibson <david@gibson.dropbear.id.au>
---
arch/powerpc/kernel/iommu.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 31c4fdc..7c309fe 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -36,6 +36,7 @@
#include <linux/hash.h>
#include <linux/fault-inject.h>
#include <linux/pci.h>
+#include <linux/kvm_host.h>
#include <asm/io.h>
#include <asm/prom.h>
#include <asm/iommu.h>
@@ -860,3 +861,19 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
free_pages((unsigned long)vaddr, get_order(size));
}
}
+
+#ifdef CONFIG_IOMMU_API
+/*
+ * SPAPR TCE API
+ */
+
+/* POWERPC does not use IOMMU API for mapping/unmapping */
+int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+ return 0;
+}
+void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+}
+
+#endif /* CONFIG_IOMMU_API */
--
1.7.10.4
^ permalink raw reply related
* [BUG] irq_dispose_mapping after irq request failure
From: Baruch Siach @ 2013-02-11 5:31 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev
Hi lkml,
The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
code snippet it its .probe:
res = devm_request_irq(&op->dev, pdata->irq,
mpc85xx_pci_isr, IRQF_DISABLED,
"[EDAC] PCI err", pci);
if (res < 0) {
irq_dispose_mapping(pdata->irq);
goto err2;
}
Now, since the requested irq is already in use, and IRQF_SHARED is not set,
devm_request_irq errors() out, which is OK. Less OK is the
irq_dispose_mapping() call, which gives me this:
EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)
mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
remove_proc_entry: removing non-empty directory 'irq/16', leaking at least '[EDAC] PCI err'
------------[ cut here ]------------
WARNING: at fs/proc/generic.c:842
NIP: c00cd00c LR: c00cd00c CTR: c000c5e4
REGS: cf039b80 TRAP: 0700 Not tainted (3.8.0-rc7-00002-g37ddebf)
MSR: 00029000 <CE,EE,ME> CR: 42042422 XER: 00000000
TASK = cf034000[1] 'swapper' THREAD: cf038000
GPR00: c00cd00c cf039c30 cf034000 0000005b 0000005c 0000005c c04b7dde 435d2050
GPR08: 43492065 c04a9a44 00000000 cf039bf0 22042424 00000000 c00025d0 00000000
GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 c042fe78
GPR24: 00000000 00000000 c04c3f90 cf05294c 00100100 00200200 cf039c78 cf052900
NIP [c00cd00c] remove_proc_entry+0x190/0x1bc
LR [c00cd00c] remove_proc_entry+0x190/0x1bc
Call Trace:
[cf039c30] [c00cd00c] remove_proc_entry+0x190/0x1bc (unreliable)
[cf039c70] [c0058c64] unregister_irq_proc+0x6c/0x74
[cf039c90] [c0054530] free_desc+0x34/0x68
[cf039cb0] [c00545f0] irq_free_descs+0x44/0x88
[cf039cd0] [c00585c8] irq_dispose_mapping+0x68/0x70
[cf039ce0] [c0222650] mpc85xx_pci_err_probe+0x2a8/0x308
[cf039d20] [c0014f8c] fsl_pci_probe+0x74/0x80
[cf039d30] [c01a9c48] platform_drv_probe+0x20/0x30
[cf039d40] [c01a88c4] driver_probe_device+0xcc/0x1f4
[cf039d60] [c01a7288] bus_for_each_drv+0x60/0x9c
[cf039d90] [c01a85ac] device_attach+0x78/0x90
[cf039db0] [c01a7430] bus_probe_device+0x34/0x9c
[cf039dd0] [c01a55c4] device_add+0x410/0x580
[cf039e10] [c022eef4] of_device_add+0x40/0x50
[cf039e20] [c022f550] of_platform_device_create_pdata+0x6c/0x8c
[cf039e40] [c022f658] of_platform_bus_create+0xe8/0x178
[cf039e90] [c022f7a0] of_platform_bus_probe+0xac/0xdc
[cf039eb0] [c0415488] mpc85xx_common_publish_devices+0x20/0x30
[cf039ec0] [c0415578] __machine_initcall_p1020_rdb_mpc85xx_common_publish_devices+0x2c/0x3c
[cf039ed0] [c040e83c] do_one_initcall+0xdc/0x1b4
[cf039f00] [c040ea24] kernel_init_freeable+0x110/0x1a8
[cf039f30] [c00025e8] kernel_init+0x18/0xf8
[cf039f40] [c000b868] ret_from_kernel_thread+0x64/0x6c
Instruction dump:
2f870000 41be0030 80bf002c 3c80c033 3c60c03e 38846be0 38840260 38a50055
38df0055 38e70055 38631770 48260331 <0fe00000> 7fe3fb78 4bfffb41 48000018
---[ end trace 9af370ce0e147530 ]---
So, is irq_dispose_mapping() the right thing to do when irq request fails?
A simple grep shows that irq_dispose_mapping() calls are mostly limited to
powerpc code. Is there a reason for that?
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Michael Ellerman @ 2013-02-11 6:19 UTC (permalink / raw)
To: Baruch Siach; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20130211053100.GB18462@sapphire.tkos.co.il>
On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:
> Hi lkml,
Hi Baruch,
> The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
> code snippet it its .probe:
You dropped an important detail which is the preceeding line:
pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);
> res = devm_request_irq(&op->dev, pdata->irq,
> mpc85xx_pci_isr, IRQF_DISABLED,
> "[EDAC] PCI err", pci);
> if (res < 0) {
> irq_dispose_mapping(pdata->irq);
> goto err2;
> }
>
> Now, since the requested irq is already in use, and IRQF_SHARED is not set,
> devm_request_irq errors() out, which is OK. Less OK is the
> irq_dispose_mapping() call, which gives me this:
>
> EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
> genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)
The hint here is to notice which other irq you're clashing with ^^
ie. yourself. Which is odd, that is the root of the problem.
The badness you're getting from irq_dispose_mapping() is caused because you're
disposing of that mapping which is currently still in use, by the same interrupt.
That is caused by a "feature" in the irq mapping code, where if you ask to map an
already mapped hwirq, it will give you back the same virq. So in your case when
you called irq_of_parse_and_map() it noticed that someone had already mapped
that hwirq, and gave you back an existing (in use) virq.
> mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
^
While you're there, can you fix the typo :)
> So, is irq_dispose_mapping() the right thing to do when irq request fails?
It's the right thing to do to undo the effect of irq_create_mapping(), or in your case irq_of_parse_and_map().
It just falls down in this case, because you're inadvertently disposing of something that's still in use.
> A simple grep shows that irq_dispose_mapping() calls are mostly limited to
> powerpc code. Is there a reason for that?
That's because the irq domain code began life as powerpc specific code. It's now become generic and will start to appear in more places.
cheers
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Baruch Siach @ 2013-02-11 6:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20130211061949.GA5561@concordia>
Hi Michael,
On Mon, Feb 11, 2013 at 05:19:49PM +1100, Michael Ellerman wrote:
> On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:
[...]
> > mpc85xx_pci_err_probe: Unable to requiest irq 16 for MPC85xx PCI err
> While you're there, can you fix the typo :)
The patch fixing it is already queued at
http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commitdiff;h=e7d2c215e56dc9fa0a01e26f2acfc3d73c889ba3.
Thanks for your details explanation. I'll now try to figure out what's wrong
with my device tree.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
^ permalink raw reply
* Re: Re[3]: PS3 platform is broken on Linux 3.7.0
From: Aneesh Kumar K.V @ 2013-02-11 10:26 UTC (permalink / raw)
To: Phileas Fogg, Geoff Levand, linuxppc-dev
In-Reply-To: <87k3qg4092.fsf@linux.vnet.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> Phileas Fogg <phileas-fogg@mail.ru> writes:
>
>> And another note.
>> I took a look at the MMU chapter in the Cell Architecture handbook and i=
ndeed the first 15 bits in VA are treated as 0 by the hardware.
>>
>> Quote:
>>
>> 1. High-order bits above 65 bits in the 80-bit virtual address (VA[0:14]=
) are not implemented. The hardware always
>> =C2=A0=C2=A0 treats these bits as `0'. Software must not set these bits =
to any other value than `0' or the results are undefined in
>> =C2=A0=C2=A0 the PPE.
>>
>>
>
> True, we missed the below part of ISA doc:
>
> ISA doc says
>
> "On implementations that support a virtual address size
> of only n bits, n < 78, bits 0:77-n of the AVA field must be
> zeros. "
>
> The Cell document I found at=20
>
> https://www-01.ibm.com/chips/techlib/techlib.nsf/techdocs/7A77CCDF14FE70D=
5852575CA0074E8ED/$file/CellBE_Handbook_v1.12_3Apr09_pub.pdf
>
> gives=20
>
> Virtual Address (VA) Size -> 65 bits
>
> So as per ISA, bits 0:12 should be zero, which should make 0:14 of PTE
> fields zero for Cell.
>
> I will try to do a patch.=20
>
Can you try this patch ?
diff --git a/arch/powerpc/include/asm/mmu-hash64.h b/arch/powerpc/include/a=
sm/mmu-hash64.h
index 2fdb47a..f01fd9a 100644
--- a/arch/powerpc/include/asm/mmu-hash64.h
+++ b/arch/powerpc/include/asm/mmu-hash64.h
@@ -381,21 +381,37 @@ extern void slb_set_size(u16 size);
* hash collisions.
*/
=20
+/* This should go in Kconfig */
+/*
+ * Be careful with this value. This determines the VSID_MODULUS_* and that
+ * need to be co-prime with VSID_MULTIPLIER*
+ */
+#if 1
+#define MAX_VIRTUAL_ADDR_BITS 65
+#else
+#define MAX_VIRTUAL_ADDR_BITS 66
+#endif
+/*
+ * One bit is taken by the kernel, only the rest of space is available for=
the
+ * user space.
+ */
+#define CONTEXT_BITS (MAX_VIRTUAL_ADDR_BITS - \
+ (USER_ESID_BITS + SID_SHIFT + 1))
+#define USER_ESID_BITS 18
+#define USER_ESID_BITS_1T 6
+
/*
* This should be computed such that protovosid * vsid_mulitplier
* doesn't overflow 64 bits. It should also be co-prime to vsid_modulus
*/
#define VSID_MULTIPLIER_256M ASM_CONST(12538073) /* 24-bit prime */
-#define VSID_BITS_256M 38
+#define VSID_BITS_256M (CONTEXT_BITS + USER_ESID_BITS + 1)
#define VSID_MODULUS_256M ((1UL<<VSID_BITS_256M)-1)
=20
#define VSID_MULTIPLIER_1T ASM_CONST(12538073) /* 24-bit prime */
-#define VSID_BITS_1T 26
+#define VSID_BITS_1T (CONTEXT_BITS + USER_ESID_BITS_1T + 1)
#define VSID_MODULUS_1T ((1UL<<VSID_BITS_1T)-1)
=20
-#define CONTEXT_BITS 19
-#define USER_ESID_BITS 18
-#define USER_ESID_BITS_1T 6
=20
#define USER_VSID_RANGE (1UL << (USER_ESID_BITS + SID_SHIFT))
=20
^ permalink raw reply related
* Re: [PATCH] Centralise CONFIG_ARCH_NO_VIRT_TO_BUS
From: James Hogan @ 2013-02-11 11:57 UTC (permalink / raw)
To: Stephen Rothwell
Cc: linux-arch, linux-sh, Vineet Gupta, linux-kernel, David S. Miller,
sparclinux, Paul Mundt, Paul Mackerras, Bjorn Helgaas,
Andrew Morton, linuxppc-dev, H Hartley Sweeten
In-Reply-To: <20121113082615.2f482eb8835daf46e1f27947@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 364 bytes --]
Hi Stephen,
On 12/11/12 21:26, Stephen Rothwell wrote:
> Make if easier for more architectures to select it and thus disable
> drivers that use virt_to_bus().
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
I was just wondering what the status of this patch is? It was in -next
for a while but seems to have disappeared.
Cheers
James
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v5 00/45] CPU hotplug: stop_machine()-free CPU hotplug
From: Vincent Guittot @ 2013-02-11 11:58 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, linux-kernel, walken, mingo,
linux-arch, Russell King - ARM Linux, xiaoguangrong, wangyun,
paulmck, nikunj, linux-pm, Rusty Russell, rostedt, rjw, namhyung,
tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm, linuxppc-dev
In-Reply-To: <51153F72.1060005@linux.vnet.ibm.com>
Hi Srivatsa,
I can try to run some of our stress tests on your patches. Have you
got a git tree that i can pull ?
Regards,
Vincent
On 8 February 2013 19:09, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 02/08/2013 10:14 PM, Srivatsa S. Bhat wrote:
>> On 02/08/2013 09:11 PM, Russell King - ARM Linux wrote:
>>> On Thu, Feb 07, 2013 at 11:41:34AM +0530, Srivatsa S. Bhat wrote:
>>>> On 02/07/2013 09:44 AM, Rusty Russell wrote:
>>>>> "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> writes:
>>>>>> On 01/22/2013 01:03 PM, Srivatsa S. Bhat wrote:
>>>>>> Avg. latency of 1 CPU offline (ms) [stop-cpu/stop-m/c latency]
>>>>>>
>>>>>> # online CPUs Mainline (with stop-m/c) This patchset (no stop-m/c)
>>>>>>
>>>>>> 8 17.04 7.73
>>>>>>
>>>>>> 16 18.05 6.44
>>>>>>
>>>>>> 32 17.31 7.39
>>>>>>
>>>>>> 64 32.40 9.28
>>>>>>
>>>>>> 128 98.23 7.35
>>>>>
>>>>> Nice!
>>>>
>>>> Thank you :-)
>>>>
>>>>> I wonder how the ARM guys feel with their quad-cpu systems...
>>>>>
>>>>
>>>> That would be definitely interesting to know :-)
>>>
>>> That depends what exactly you'd like tested (and how) and whether you'd
>>> like it to be a test-chip based quad core, or an OMAP dual-core SoC.
>>>
>>
>> The effect of stop_machine() doesn't really depend on the CPU architecture
>> used underneath or the platform. It depends only on the _number_ of
>> _logical_ CPUs used.
>>
>> And stop_machine() has 2 noticeable drawbacks:
>> 1. It makes the hotplug operation itself slow
>> 2. and it causes disruptions to the workloads running on the other
>> CPUs by hijacking the entire machine for significant amounts of time.
>>
>> In my experiments (mentioned above), I tried to measure how my patchset
>> improves (reduces) the duration of hotplug (CPU offline) itself. Which is
>> also slightly indicative of the impact it has on the rest of the system.
>>
>> But what would be nice to test, is a setup where the workloads running on
>> the rest of the system are latency-sensitive, and measure the impact of
>> CPU offline on them, with this patchset applied. That would tell us how
>> far is this useful in making CPU hotplug less disruptive on the system.
>>
>> Of course, it would be nice to also see whether we observe any reduction
>> in hotplug duration itself (point 1 above) on ARM platforms with lot
>> of CPUs. [This could potentially speed up suspend/resume, which is used
>> rather heavily on ARM platforms].
>>
>> The benefits from this patchset over mainline (both in terms of points
>> 1 and 2 above) is expected to increase, with increasing number of CPUs in
>> the system.
>>
>
> Adding Vincent to CC, who had previously evaluated the performance and
> latency implications of CPU hotplug on ARM platforms, IIRC.
>
> Regards,
> Srivatsa S. Bhat
>
^ permalink raw reply
* [PATCH 0/2] vfio on power
From: Alexey Kardashevskiy @ 2013-02-11 11:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: kvm, Alexey Kardashevskiy, linux-kernel, Alex Williamson,
Paul Mackerras, linuxppc-dev, David Gibson
The series introduces a VFIO support on POWER.
The QEMU support is required, the real mode acceleration patches are coming later.
Alexey Kardashevskiy (2):
vfio powerpc: enabled on powernv platform
vfio powerpc: implemented IOMMU driver for VFIO
arch/powerpc/include/asm/iommu.h | 15 ++
arch/powerpc/kernel/iommu.c | 343 +++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
arch/powerpc/platforms/powernv/pci-p5ioc2.c | 5 +-
arch/powerpc/platforms/powernv/pci.c | 3 +
drivers/iommu/Kconfig | 8 +
drivers/vfio/Kconfig | 6 +
drivers/vfio/Makefile | 1 +
drivers/vfio/vfio_iommu_spapr_tce.c | 269 +++++++++++++++++++++
include/uapi/linux/vfio.h | 31 +++
10 files changed, 681 insertions(+), 1 deletion(-)
create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
--
1.7.10.4
^ permalink raw reply
* [PATCH 2/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alexey Kardashevskiy @ 2013-02-11 11:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: kvm, Alexey Kardashevskiy, linux-kernel, Alex Williamson,
Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360583672-21924-1-git-send-email-aik@ozlabs.ru>
VFIO implements platform independent stuff such as
a PCI driver, BAR access (via read/write on a file descriptor
or direct mapping when possible) and IRQ signaling.
The platform dependent part includes IOMMU initialization
and handling. This patch implements an IOMMU driver for VFIO
which does mapping/unmapping pages for the guest IO and
provides information about DMA window (required by a POWERPC
guest).
The counterpart in QEMU is required to support this functionality.
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
drivers/vfio/Kconfig | 6 +
drivers/vfio/Makefile | 1 +
drivers/vfio/vfio_iommu_spapr_tce.c | 269 +++++++++++++++++++++++++++++++++++
include/uapi/linux/vfio.h | 31 ++++
4 files changed, 307 insertions(+)
create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 7cd5dec..b464687 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
depends on VFIO
default n
+config VFIO_IOMMU_SPAPR_TCE
+ tristate
+ depends on VFIO && SPAPR_TCE_IOMMU
+ default n
+
menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
select VFIO_IOMMU_TYPE1 if X86
+ select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/vfio.txt for more details.
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 2398d4a..72bfabc 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_VFIO) += vfio.o
obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
+obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
obj-$(CONFIG_VFIO_PCI) += pci/
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
new file mode 100644
index 0000000..9b3fa88
--- /dev/null
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -0,0 +1,269 @@
+/*
+ * VFIO: IOMMU DMA mapping support for TCE on POWER
+ *
+ * Copyright (C) 2012 IBM Corp. All rights reserved.
+ * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Derived from original vfio_iommu_type1.c:
+ * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
+ * Author: Alex Williamson <alex.williamson@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/err.h>
+#include <linux/vfio.h>
+#include <asm/iommu.h>
+#include <asm/tce.h>
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "aik@ozlabs.ru"
+#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
+
+static void tce_iommu_detach_group(void *iommu_data,
+ struct iommu_group *iommu_group);
+
+/*
+ * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
+ *
+ * This code handles mapping and unmapping of user data buffers
+ * into DMA'ble space using the IOMMU
+ */
+
+/*
+ * The container descriptor supports only a single group per container.
+ * Required by the API as the container is not supplied with the IOMMU group
+ * at the moment of initialization.
+ */
+struct tce_container {
+ struct mutex lock;
+ struct iommu_table *tbl;
+};
+
+static void *tce_iommu_open(unsigned long arg)
+{
+ struct tce_container *container;
+
+ if (arg != VFIO_SPAPR_TCE_IOMMU) {
+ pr_err("tce_vfio: Wrong IOMMU type\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ container = kzalloc(sizeof(*container), GFP_KERNEL);
+ if (!container)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_init(&container->lock);
+
+ return container;
+}
+
+static void tce_iommu_release(void *iommu_data)
+{
+ struct tce_container *container = iommu_data;
+
+ WARN_ON(container->tbl && !container->tbl->it_group);
+ if (container->tbl && container->tbl->it_group)
+ tce_iommu_detach_group(iommu_data, container->tbl->it_group);
+
+ mutex_destroy(&container->lock);
+
+ kfree(container);
+}
+
+static long tce_iommu_ioctl(void *iommu_data,
+ unsigned int cmd, unsigned long arg)
+{
+ struct tce_container *container = iommu_data;
+ unsigned long minsz;
+ long ret;
+
+ switch (cmd) {
+ case VFIO_CHECK_EXTENSION:
+ return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
+
+ case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
+ struct vfio_iommu_spapr_tce_info info;
+ struct iommu_table *tbl = container->tbl;
+
+ if (WARN_ON(!tbl))
+ return -ENXIO;
+
+ minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
+ dma32_window_size);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
+ info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
+ info.flags = 0;
+
+ if (copy_to_user((void __user *)arg, &info, minsz))
+ return -EFAULT;
+
+ return 0;
+ }
+ case VFIO_IOMMU_MAP_DMA: {
+ vfio_iommu_spapr_tce_dma_map param;
+ struct iommu_table *tbl = container->tbl;
+ unsigned long tce;
+
+ if (WARN_ON(!tbl))
+ return -ENXIO;
+
+ minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
+
+ if (copy_from_user(¶m, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (param.argsz < minsz)
+ return -EINVAL;
+
+ if (param.flags & ~(VFIO_DMA_MAP_FLAG_READ |
+ VFIO_DMA_MAP_FLAG_WRITE))
+ return -EINVAL;
+
+ if ((param.size & ~IOMMU_PAGE_MASK) ||
+ (param.vaddr & ~IOMMU_PAGE_MASK))
+ return -EINVAL;
+
+ /* TODO: support multiple TCEs */
+ if (param.size != IOMMU_PAGE_SIZE) {
+ pr_err("VFIO map on POWER supports only %lu page size\n",
+ IOMMU_PAGE_SIZE);
+ return -EINVAL;
+ }
+
+ /* iova is checked by the IOMMU API */
+ tce = param.vaddr;
+ if (param.flags & VFIO_DMA_MAP_FLAG_READ)
+ tce |= TCE_PCI_READ;
+ if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
+ tce |= TCE_PCI_WRITE;
+
+ ret = iommu_put_tce_user_mode(tbl, param.iova, tce);
+ iommu_flush_tce(tbl);
+
+ return ret;
+ }
+ case VFIO_IOMMU_UNMAP_DMA: {
+ vfio_iommu_spapr_tce_dma_unmap param;
+ struct iommu_table *tbl = container->tbl;
+
+ if (WARN_ON(!tbl))
+ return -ENXIO;
+
+ minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
+
+ if (copy_from_user(¶m, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (param.argsz < minsz)
+ return -EINVAL;
+
+ /* No flag is supported now */
+ if (param.flags)
+ return -EINVAL;
+
+ if (param.size & ~IOMMU_PAGE_MASK)
+ return -EINVAL;
+
+ /* iova is checked by the IOMMU API */
+ ret = iommu_clear_tce_user_mode(tbl, param.iova, 0,
+ param.size >> IOMMU_PAGE_SHIFT);
+ iommu_flush_tce(tbl);
+
+ return ret;
+ }
+ }
+
+ return -ENOTTY;
+}
+
+static int tce_iommu_attach_group(void *iommu_data,
+ struct iommu_group *iommu_group)
+{
+ int ret;
+ struct tce_container *container = iommu_data;
+ struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
+
+ BUG_ON(!tbl);
+ mutex_lock(&container->lock);
+ pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
+ iommu_group_id(iommu_group), iommu_group);
+ if (container->tbl) {
+ pr_warn("tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
+ iommu_group_id(container->tbl->it_group),
+ iommu_group_id(iommu_group));
+ mutex_unlock(&container->lock);
+ return -EBUSY;
+ }
+
+ container->tbl = tbl;
+ ret = iommu_lock_table(tbl, true);
+ mutex_unlock(&container->lock);
+
+ return ret;
+}
+
+static void tce_iommu_detach_group(void *iommu_data,
+ struct iommu_group *iommu_group)
+{
+ struct tce_container *container = iommu_data;
+ struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
+
+ BUG_ON(!tbl);
+ mutex_lock(&container->lock);
+ if (tbl != container->tbl) {
+ pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n",
+ iommu_group_id(iommu_group),
+ iommu_group_id(tbl->it_group));
+ } else {
+
+ pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
+ iommu_group_id(iommu_group), iommu_group);
+
+ container->tbl = NULL;
+ iommu_lock_table(tbl, false);
+ }
+ mutex_unlock(&container->lock);
+}
+
+const struct vfio_iommu_driver_ops tce_iommu_driver_ops = {
+ .name = "iommu-vfio-powerpc",
+ .owner = THIS_MODULE,
+ .open = tce_iommu_open,
+ .release = tce_iommu_release,
+ .ioctl = tce_iommu_ioctl,
+ .attach_group = tce_iommu_attach_group,
+ .detach_group = tce_iommu_detach_group,
+};
+
+static int __init tce_iommu_init(void)
+{
+ return vfio_register_iommu_driver(&tce_iommu_driver_ops);
+}
+
+static void __exit tce_iommu_cleanup(void)
+{
+ vfio_unregister_iommu_driver(&tce_iommu_driver_ops);
+}
+
+module_init(tce_iommu_init);
+module_exit(tce_iommu_cleanup);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 4758d1b..ea9a9a7 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -22,6 +22,7 @@
/* Extensions */
#define VFIO_TYPE1_IOMMU 1
+#define VFIO_SPAPR_TCE_IOMMU 2
/*
* The IOCTL interface is designed for extensibility by embedding the
@@ -365,4 +366,34 @@ struct vfio_iommu_type1_dma_unmap {
#define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
+/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
+
+/*
+ * The SPAPR TCE info struct provides the information about the PCI bus
+ * address ranges available for DMA, these values are programmed into
+ * the hardware so the guest has to know that information.
+ *
+ * The DMA 32 bit window start is an absolute PCI bus address.
+ * The IOVA address passed via map/unmap ioctls are absolute PCI bus
+ * addresses too so the window works as a filter rather than an offset
+ * for IOVA addresses.
+ *
+ * A flag will need to be added if other page sizes are supported,
+ * so as defined here, it is always 4k.
+ */
+struct vfio_iommu_spapr_tce_info {
+ __u32 argsz;
+ __u32 flags; /* reserved for future use */
+ __u32 dma32_window_start; /* 32 bit window start (bytes) */
+ __u32 dma32_window_size; /* 32 bit window size (bytes) */
+};
+
+#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
+
+/* Reuse type1 map/unmap structs as they are the same at the moment */
+typedef struct vfio_iommu_type1_dma_map vfio_iommu_spapr_tce_dma_map;
+typedef struct vfio_iommu_type1_dma_unmap vfio_iommu_spapr_tce_dma_unmap;
+
+/* ***************************************************************** */
+
#endif /* _UAPIVFIO_H */
--
1.7.10.4
^ permalink raw reply related
* [PATCH 1/4] powerpc: lookup_linux_pte has been made public
From: aik @ 2013-02-11 12:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: kvm, Alexey Kardashevskiy, Alexander Graf, kvm-ppc, linux-kernel,
Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360584763-21988-1-git-send-email-a>
From: Alexey Kardashevskiy <aik@ozlabs.ru>
The lookup_linux_pte() function returns a linux PTE which
is required to convert KVM guest physical address into host real
address in real mode.
This convertion will be used by upcoming support of H_PUT_TCE_INDIRECT
as TCE list address comes from the guest directly so it is a guest
physical.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: David Gibson <david@gibson.dropbear.id.au>
---
arch/powerpc/include/asm/pgtable-ppc64.h | 3 +++
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/pgtable-ppc64.h b/arch/powerpc/include/asm/pgtable-ppc64.h
index 0182c20..ddcc898 100644
--- a/arch/powerpc/include/asm/pgtable-ppc64.h
+++ b/arch/powerpc/include/asm/pgtable-ppc64.h
@@ -377,6 +377,9 @@ static inline pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
}
#endif /* !CONFIG_HUGETLB_PAGE */
+pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
+ int writing, unsigned long *pte_sizep);
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_PGTABLE_PPC64_H_ */
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 19c93ba..6a042d0 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -145,8 +145,8 @@ static void remove_revmap_chain(struct kvm *kvm, long pte_index,
unlock_rmap(rmap);
}
-static pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
- int writing, unsigned long *pte_sizep)
+pte_t lookup_linux_pte(pgd_t *pgdir, unsigned long hva,
+ int writing, unsigned long *pte_sizep)
{
pte_t *ptep;
unsigned long ps = *pte_sizep;
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/4] powerpc kvm: added multiple TCEs requests support
From: aik @ 2013-02-11 12:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: kvm, Alexey Kardashevskiy, Alexander Graf, kvm-ppc, linux-kernel,
Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360584763-21988-1-git-send-email-a>
From: Alexey Kardashevskiy <aik@ozlabs.ru>
The patch adds real mode handlers for H_PUT_TCE_INDIRECT and
H_STUFF_TCE hypercalls for QEMU emulated devices such as virtio
devices or emulated PCI. These calls allow adding multiple entries
(up to 512) into the TCE table in one call which saves time on
transition to/from real mode.
The patch adds a guest physical to host real address converter
and calls the existing H_PUT_TCE handler. The converting function
is going to be fully utilized by upcoming VFIO supporting patches.
The patch also implements the KVM_CAP_PPC_MULTITCE capability,
so in order to support the functionality of this patch QEMU
needs to query for this capability and set the "hcall-multi-tce"
hypertas property if the capability is present.
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/include/asm/kvm_ppc.h | 15 ++
arch/powerpc/kvm/book3s_64_vio_hv.c | 241 ++++++++++++++++++++++++++++---
arch/powerpc/kvm/book3s_hv.c | 23 +++
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 6 +
arch/powerpc/kvm/book3s_pr_papr.c | 37 ++++-
arch/powerpc/kvm/powerpc.c | 3 +
include/uapi/linux/kvm.h | 1 +
7 files changed, 301 insertions(+), 25 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 572aa75..76d133b 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -136,6 +136,21 @@ extern long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
struct kvm_create_spapr_tce *args);
extern long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
unsigned long ioba, unsigned long tce);
+extern long kvmppc_h_put_tce_indirect(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_list, unsigned long npages);
+extern long kvmppc_h_stuff_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_value, unsigned long npages);
+extern long kvmppc_virtmode_h_put_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce);
+extern long kvmppc_virtmode_h_put_tce_indirect(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_list, unsigned long npages);
+extern long kvmppc_virtmode_h_stuff_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_value, unsigned long npages);
extern long kvm_vm_ioctl_allocate_rma(struct kvm *kvm,
struct kvm_allocate_rma *rma);
extern struct kvmppc_linear_info *kvm_alloc_rma(void);
diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
index 30c2f3b..c38edcd 100644
--- a/arch/powerpc/kvm/book3s_64_vio_hv.c
+++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
@@ -14,6 +14,7 @@
*
* Copyright 2010 Paul Mackerras, IBM Corp. <paulus@au1.ibm.com>
* Copyright 2011 David Gibson, IBM Corporation <dwg@au1.ibm.com>
+ * Copyright 2013 Alexey Kardashevskiy, IBM Corporation <aik@au1.ibm.com>
*/
#include <linux/types.h>
@@ -25,6 +26,7 @@
#include <linux/slab.h>
#include <linux/hugetlb.h>
#include <linux/list.h>
+#include <linux/kvm_host.h>
#include <asm/tlbflush.h>
#include <asm/kvm_ppc.h>
@@ -35,42 +37,233 @@
#include <asm/ppc-opcode.h>
#include <asm/kvm_host.h>
#include <asm/udbg.h>
+#include <asm/iommu.h>
+#include <asm/tce.h>
#define TCES_PER_PAGE (PAGE_SIZE / sizeof(u64))
-/* WARNING: This will be called in real-mode on HV KVM and virtual
- * mode on PR KVM
+static struct kvmppc_spapr_tce_table *find_tce_table(struct kvm_vcpu *vcpu,
+ unsigned long liobn)
+{
+ struct kvmppc_spapr_tce_table *stt;
+
+ list_for_each_entry(stt, &vcpu->kvm->arch.spapr_tce_tables, list) {
+ if (stt->liobn == liobn)
+ return stt;
+ }
+
+ return NULL;
+}
+
+/*
+ * Converts guest physical address into host virtual
+ * which is to be used later in get_user_pages_fast().
+ */
+static unsigned long get_virt_address(struct kvm_vcpu *vcpu,
+ unsigned long gpa, bool writing,
+ pte_t *ptep, unsigned long *pg_sizep)
+{
+ unsigned long hva, gfn = gpa >> PAGE_SHIFT;
+ struct kvm_memory_slot *memslot;
+
+ memslot = search_memslots(kvm_memslots(vcpu->kvm), gfn);
+ if (!memslot)
+ return 0;
+
+ /*
+ * Convert gfn to hva preserving flags and an offset
+ * within a system page
+ */
+ hva = __gfn_to_hva_memslot(memslot, gfn) + (gpa & ~PAGE_MASK);
+
+ /* Find out the page pte and size if requested */
+ if (ptep && pg_sizep) {
+ pte_t pte;
+ unsigned long pg_size = 0;
+
+ pte = lookup_linux_pte(vcpu->arch.pgdir, hva,
+ writing, &pg_size);
+ if (!pte_present(pte))
+ return 0;
+
+ *pg_sizep = pg_size;
+ *ptep = pte;
+ }
+
+ return hva;
+}
+
+/*
+ * Converts guest physical address into host real address.
+ * Also returns pte and page size if the page is present in page table.
+ */
+static unsigned long get_real_address(struct kvm_vcpu *vcpu,
+ unsigned long gpa, bool writing,
+ pte_t *ptep, unsigned long *pg_sizep)
+{
+ struct kvm_memory_slot *memslot;
+ pte_t pte;
+ unsigned long hva, pg_size = 0, hwaddr, offset;
+ unsigned long gfn = gpa >> PAGE_SHIFT;
+
+ /* Find a KVM memslot */
+ memslot = search_memslots(kvm_memslots(vcpu->kvm), gfn);
+ if (!memslot)
+ return 0;
+
+ /* Convert guest physical address to host virtual */
+ hva = __gfn_to_hva_memslot(memslot, gfn);
+
+ /* Find a PTE and determine the size */
+ pte = lookup_linux_pte(vcpu->arch.pgdir, hva,
+ writing, &pg_size);
+ if (!pte_present(pte))
+ return 0;
+
+ /* Calculate host phys address keeping flags and offset in the page */
+ offset = gpa & (pg_size - 1);
+
+ /* pte_pfn(pte) should return an address aligned to pg_size */
+ hwaddr = (pte_pfn(pte) << PAGE_SHIFT) + offset;
+
+ /* Copy outer values if required */
+ if (pg_sizep)
+ *pg_sizep = pg_size;
+ if (ptep)
+ *ptep = pte;
+
+ return hwaddr;
+}
+
+/*
+ * emulated_h_put_tce() handles TCE requests for devices emulated
+ * by QEMU. It puts guest TCE values into the table and expects
+ * the QEMU to convert them later in the QEMU device implementation.
+ * Works in both real and virtual modes.
+ */
+static long emulated_h_put_tce(struct kvmppc_spapr_tce_table *stt,
+ unsigned long ioba, unsigned long tce)
+{
+ unsigned long idx = ioba >> SPAPR_TCE_SHIFT;
+ struct page *page;
+ u64 *tbl;
+
+ /* udbg_printf("H_PUT_TCE: liobn 0x%lx => stt=%p window_size=0x%x\n", */
+ /* liobn, stt, stt->window_size); */
+ if (ioba >= stt->window_size) {
+ pr_err("%s failed on ioba=%lx\n", __func__, ioba);
+ return H_PARAMETER;
+ }
+
+ page = stt->pages[idx / TCES_PER_PAGE];
+ tbl = (u64 *)page_address(page);
+
+ /* FIXME: Need to validate the TCE itself */
+ /* udbg_printf("tce @ %p\n", &tbl[idx % TCES_PER_PAGE]); */
+ tbl[idx % TCES_PER_PAGE] = tce;
+
+ return H_SUCCESS;
+}
+
+/*
+ * Real mode handlers
*/
long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
unsigned long ioba, unsigned long tce)
{
- struct kvm *kvm = vcpu->kvm;
struct kvmppc_spapr_tce_table *stt;
- /* udbg_printf("H_PUT_TCE(): liobn=0x%lx ioba=0x%lx, tce=0x%lx\n", */
- /* liobn, ioba, tce); */
+ stt = find_tce_table(vcpu, liobn);
+ /* Didn't find the liobn, put it to userspace */
+ if (!stt)
+ return H_TOO_HARD;
+
+ /* Emulated IO */
+ return emulated_h_put_tce(stt, ioba, tce);
+}
+
+long kvmppc_h_put_tce_indirect(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_list, unsigned long npages)
+{
+ struct kvmppc_spapr_tce_table *stt;
+ long i, ret = 0;
+ unsigned long *tces;
+
+ stt = find_tce_table(vcpu, liobn);
+ /* Didn't find the liobn, put it to userspace */
+ if (!stt)
+ return H_TOO_HARD;
- list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
- if (stt->liobn == liobn) {
- unsigned long idx = ioba >> SPAPR_TCE_SHIFT;
- struct page *page;
- u64 *tbl;
+ tces = (void *) get_real_address(vcpu, tce_list, false, NULL, NULL);
+ if (!tces)
+ return H_TOO_HARD;
- /* udbg_printf("H_PUT_TCE: liobn 0x%lx => stt=%p window_size=0x%x\n", */
- /* liobn, stt, stt->window_size); */
- if (ioba >= stt->window_size)
- return H_PARAMETER;
+ /* Emulated IO */
+ for (i = 0; (i < npages) && !ret; ++i, ioba += IOMMU_PAGE_SIZE)
+ ret = emulated_h_put_tce(stt, ioba, tces[i]);
- page = stt->pages[idx / TCES_PER_PAGE];
- tbl = (u64 *)page_address(page);
+ return ret;
+}
- /* FIXME: Need to validate the TCE itself */
- /* udbg_printf("tce @ %p\n", &tbl[idx % TCES_PER_PAGE]); */
- tbl[idx % TCES_PER_PAGE] = tce;
- return H_SUCCESS;
- }
- }
+long kvmppc_h_stuff_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_value, unsigned long npages)
+{
+ struct kvmppc_spapr_tce_table *stt;
+ long i, ret = 0;
+
+ stt = find_tce_table(vcpu, liobn);
+ /* Didn't find the liobn, put it to userspace */
+ if (!stt)
+ return H_TOO_HARD;
- /* Didn't find the liobn, punt it to userspace */
- return H_TOO_HARD;
+ /* Emulated IO */
+ for (i = 0; (i < npages) && !ret; ++i, ioba += IOMMU_PAGE_SIZE)
+ ret = emulated_h_put_tce(stt, ioba, tce_value);
+
+ return ret;
+}
+
+/*
+ * Virtual mode handlers
+ */
+extern long kvmppc_virtmode_h_put_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce)
+{
+ /* At the moment emulated IO is handled the same way */
+ return kvmppc_h_put_tce(vcpu, liobn, ioba, tce);
+}
+
+extern long kvmppc_virtmode_h_put_tce_indirect(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_list, unsigned long npages)
+{
+ struct kvmppc_spapr_tce_table *stt;
+ unsigned long *tces;
+ long ret = 0, i;
+
+ stt = find_tce_table(vcpu, liobn);
+ /* Didn't find the liobn, put it to userspace */
+ if (!stt)
+ return H_TOO_HARD;
+
+ tces = (void *) get_virt_address(vcpu, tce_list, false, NULL, NULL);
+ if (!tces)
+ return H_TOO_HARD;
+
+ /* Emulated IO */
+ for (i = 0; (i < npages) && !ret; ++i, ioba += IOMMU_PAGE_SIZE)
+ ret = emulated_h_put_tce(stt, ioba, tces[i]);
+
+ return ret;
+}
+
+extern long kvmppc_virtmode_h_stuff_tce(struct kvm_vcpu *vcpu,
+ unsigned long liobn, unsigned long ioba,
+ unsigned long tce_value, unsigned long npages)
+{
+ /* At the moment emulated IO is handled the same way */
+ return kvmppc_h_stuff_tce(vcpu, liobn, ioba, tce_value, npages);
}
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 71d0c90..13c8436 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -515,6 +515,29 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
kvmppc_get_gpr(vcpu, 5),
kvmppc_get_gpr(vcpu, 6));
break;
+ case H_PUT_TCE:
+ ret = kvmppc_virtmode_h_put_tce(vcpu, kvmppc_get_gpr(vcpu, 4),
+ kvmppc_get_gpr(vcpu, 5),
+ kvmppc_get_gpr(vcpu, 6));
+ if (ret == H_TOO_HARD)
+ return RESUME_HOST;
+ break;
+ case H_PUT_TCE_INDIRECT:
+ ret = kvmppc_virtmode_h_put_tce_indirect(vcpu, kvmppc_get_gpr(vcpu, 4),
+ kvmppc_get_gpr(vcpu, 5),
+ kvmppc_get_gpr(vcpu, 6),
+ kvmppc_get_gpr(vcpu, 7));
+ if (ret == H_TOO_HARD)
+ return RESUME_HOST;
+ break;
+ case H_STUFF_TCE:
+ ret = kvmppc_virtmode_h_stuff_tce(vcpu, kvmppc_get_gpr(vcpu, 4),
+ kvmppc_get_gpr(vcpu, 5),
+ kvmppc_get_gpr(vcpu, 6),
+ kvmppc_get_gpr(vcpu, 7));
+ if (ret == H_TOO_HARD)
+ return RESUME_HOST;
+ break;
default:
return RESUME_HOST;
}
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 10b6c35..0826e8b 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1390,6 +1390,12 @@ hcall_real_table:
.long 0 /* 0x11c */
.long 0 /* 0x120 */
.long .kvmppc_h_bulk_remove - hcall_real_table
+ .long 0 /* 0x128 */
+ .long 0 /* 0x12c */
+ .long 0 /* 0x130 */
+ .long 0 /* 0x134 */
+ .long .kvmppc_h_stuff_tce - hcall_real_table
+ .long .kvmppc_h_put_tce_indirect - hcall_real_table
hcall_real_table_end:
ignore_hdec:
diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c
index ee02b30..270e88e 100644
--- a/arch/powerpc/kvm/book3s_pr_papr.c
+++ b/arch/powerpc/kvm/book3s_pr_papr.c
@@ -220,7 +220,38 @@ static int kvmppc_h_pr_put_tce(struct kvm_vcpu *vcpu)
unsigned long tce = kvmppc_get_gpr(vcpu, 6);
long rc;
- rc = kvmppc_h_put_tce(vcpu, liobn, ioba, tce);
+ rc = kvmppc_virtmode_h_put_tce(vcpu, liobn, ioba, tce, 1);
+ if (rc == H_TOO_HARD)
+ return EMULATE_FAIL;
+ kvmppc_set_gpr(vcpu, 3, rc);
+ return EMULATE_DONE;
+}
+
+static int kvmppc_h_pr_put_tce_indirect(struct kvm_vcpu *vcpu)
+{
+ unsigned long liobn = kvmppc_get_gpr(vcpu, 4);
+ unsigned long ioba = kvmppc_get_gpr(vcpu, 5);
+ unsigned long tce = kvmppc_get_gpr(vcpu, 6);
+ unsigned long npages = kvmppc_get_gpr(vcpu, 7);
+ long rc;
+
+ rc = kvmppc_virtmode_h_put_tce_indirect(vcpu, liobn, ioba,
+ tce, npages);
+ if (rc == H_TOO_HARD)
+ return EMULATE_FAIL;
+ kvmppc_set_gpr(vcpu, 3, rc);
+ return EMULATE_DONE;
+}
+
+static int kvmppc_h_pr_stuff_tce(struct kvm_vcpu *vcpu)
+{
+ unsigned long liobn = kvmppc_get_gpr(vcpu, 4);
+ unsigned long ioba = kvmppc_get_gpr(vcpu, 5);
+ unsigned long tce_value = kvmppc_get_gpr(vcpu, 6);
+ unsigned long npages = kvmppc_get_gpr(vcpu, 7);
+ long rc;
+
+ rc = kvmppc_virtmode_h_stuff_tce(vcpu, liobn, ioba, tce_value, npages);
if (rc == H_TOO_HARD)
return EMULATE_FAIL;
kvmppc_set_gpr(vcpu, 3, rc);
@@ -240,6 +271,10 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
return kvmppc_h_pr_bulk_remove(vcpu);
case H_PUT_TCE:
return kvmppc_h_pr_put_tce(vcpu);
+ case H_PUT_TCE_INDIRECT:
+ return kvmppc_h_pr_put_tce_indirect(vcpu);
+ case H_STUFF_TCE:
+ return kvmppc_h_pr_stuff_tce(vcpu);
case H_CEDE:
vcpu->arch.shared->msr |= MSR_EE;
kvm_vcpu_block(vcpu);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 70739a0..95614c7 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -383,6 +383,9 @@ int kvm_dev_ioctl_check_extension(long ext)
r = 1;
break;
#endif
+ case KVM_CAP_PPC_MULTITCE:
+ r = 1;
+ break;
default:
r = 0;
break;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index e6e5d4b..26e2b271 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -635,6 +635,7 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_IRQFD_RESAMPLE 82
#define KVM_CAP_PPC_BOOKE_WATCHDOG 83
#define KVM_CAP_PPC_HTAB_FD 84
+#define KVM_CAP_PPC_MULTITCE 87
#ifdef KVM_CAP_IRQ_ROUTING
--
1.7.10.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox