* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-12 13:57 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <CA+55aFynbkeuUGs9s-q+fLY6MeRBA6MjEyWWbbe7A5AaqsAknw@mail.gmail.com>
On Mon, Nov 02, 2015 at 04:06:46PM -0800, Linus Torvalds wrote:
> On Mon, Nov 2, 2015 at 12:15 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> > So I ran some experiments on an IvyBridge (2.8GHz) and the cost of XCHG is
> > constantly cheaper (by at least half the latency) than MFENCE. While there
> > was a decent amount of variation, this difference remained rather constant.
>
> Mind testing "lock addq $0,0(%rsp)" instead of mfence? That's what we
> use on old cpu's without one (ie 32-bit).
>
> I'm not actually convinced that mfence is necessarily a good idea. I
> could easily see it being microcode, for example.
>
> At least on my Haswell, the "lock addq" is pretty much exactly half
> the cost of "mfence".
>
> Linus
mfence was high on some traces I was seeing, so I got curious, too:
---->
main.c
---->
extern volatile int x;
volatile int x;
#ifdef __x86_64__
#define SP "rsp"
#else
#define SP "esp"
#endif
#ifdef lock
#define barrier() asm("lock; addl $0,0(%%" SP ")" ::: "memory")
#endif
#ifdef xchg
#define barrier() do { int p; int ret; asm volatile ("xchgl %0, %1;": "=r"(ret) : "m"(p): "memory", "cc"); } while (0)
#endif
#ifdef xchgrz
/* same as xchg but poking at gcc red zone */
#define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
#endif
#ifdef mfence
#define barrier() asm("mfence" ::: "memory")
#endif
#ifdef lfence
#define barrier() asm("lfence" ::: "memory")
#endif
#ifdef sfence
#define barrier() asm("sfence" ::: "memory")
#endif
int main(int argc, char **argv)
{
int i;
int j = 1234;
/*
* Test barrier in a loop. We also poke at a volatile variable in an
* attempt to make it a bit more realistic - this way there's something
* in the store-buffer.
*/
for (i = 0; i < 10000000; ++i) {
x = i - j;
barrier();
j = x;
}
return 0;
}
---->
Makefile:
---->
ALL = xchg xchgrz lock mfence lfence sfence
CC = gcc
CFLAGS += -Wall -O2 -ggdb
PERF = perf stat -r 10 --log-fd 1 --
all: ${ALL}
clean:
rm -f ${ALL}
run: all
for file in ${ALL}; do echo ${PERF} ./$$file ; ${PERF} ./$$file; done
.PHONY: all clean run
${ALL}: main.c
${CC} ${CFLAGS} -D$@ -o $@ main.c
----->
Is this a good way to test it?
E.g. on my laptop I get:
perf stat -r 10 --log-fd 1 -- ./xchg
Performance counter stats for './xchg' (10 runs):
53.236967 task-clock # 0.992 CPUs utilized ( +- 0.09% )
10 context-switches # 0.180 K/sec ( +- 1.70% )
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.691 K/sec ( +- 1.13% )
190,997,612 cycles # 3.588 GHz ( +- 0.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,654,850 instructions # 0.42 insns per cycle ( +- 0.01% )
10,122,372 branches # 190.138 M/sec ( +- 0.01% )
4,514 branch-misses # 0.04% of all branches ( +- 3.37% )
0.053642809 seconds time elapsed ( +- 0.12% )
perf stat -r 10 --log-fd 1 -- ./xchgrz
Performance counter stats for './xchgrz' (10 runs):
53.189533 task-clock # 0.997 CPUs utilized ( +- 0.22% )
0 context-switches # 0.000 K/sec
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.694 K/sec ( +- 0.75% )
190,785,621 cycles # 3.587 GHz ( +- 0.03% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,602,086 instructions # 0.42 insns per cycle ( +- 0.00% )
10,112,154 branches # 190.115 M/sec ( +- 0.01% )
3,743 branch-misses # 0.04% of all branches ( +- 4.02% )
0.053343693 seconds time elapsed ( +- 0.23% )
perf stat -r 10 --log-fd 1 -- ./lock
Performance counter stats for './lock' (10 runs):
53.096434 task-clock # 0.997 CPUs utilized ( +- 0.16% )
0 context-switches # 0.002 K/sec ( +-100.00% )
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.693 K/sec ( +- 0.98% )
190,796,621 cycles # 3.593 GHz ( +- 0.02% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,601,376 instructions # 0.42 insns per cycle ( +- 0.01% )
10,112,074 branches # 190.447 M/sec ( +- 0.01% )
3,475 branch-misses # 0.03% of all branches ( +- 1.33% )
0.053252678 seconds time elapsed ( +- 0.16% )
perf stat -r 10 --log-fd 1 -- ./mfence
Performance counter stats for './mfence' (10 runs):
126.376473 task-clock # 0.999 CPUs utilized ( +- 0.21% )
0 context-switches # 0.002 K/sec ( +- 66.67% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.289 K/sec ( +- 0.84% )
456,147,770 cycles # 3.609 GHz ( +- 0.01% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,892,416 instructions # 0.18 insns per cycle ( +- 0.00% )
10,163,220 branches # 80.420 M/sec ( +- 0.01% )
4,653 branch-misses # 0.05% of all branches ( +- 1.27% )
0.126539273 seconds time elapsed ( +- 0.21% )
perf stat -r 10 --log-fd 1 -- ./lfence
Performance counter stats for './lfence' (10 runs):
47.617861 task-clock # 0.997 CPUs utilized ( +- 0.06% )
0 context-switches # 0.002 K/sec ( +-100.00% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.764 K/sec ( +- 0.45% )
170,767,856 cycles # 3.586 GHz ( +- 0.03% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,581,607 instructions # 0.47 insns per cycle ( +- 0.00% )
10,108,508 branches # 212.284 M/sec ( +- 0.00% )
3,320 branch-misses # 0.03% of all branches ( +- 1.12% )
0.047768505 seconds time elapsed ( +- 0.07% )
perf stat -r 10 --log-fd 1 -- ./sfence
Performance counter stats for './sfence' (10 runs):
20.156676 task-clock # 0.988 CPUs utilized ( +- 0.45% )
3 context-switches # 0.159 K/sec ( +- 12.15% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.002 M/sec ( +- 0.87% )
72,212,225 cycles # 3.583 GHz ( +- 0.33% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,479,149 instructions # 1.11 insns per cycle ( +- 0.00% )
10,090,785 branches # 500.618 M/sec ( +- 0.01% )
3,626 branch-misses # 0.04% of all branches ( +- 3.59% )
0.020411208 seconds time elapsed ( +- 0.52% )
So mfence is more expensive than locked instructions/xchg, but sfence/lfence
are slightly faster, and xchg and locked instructions are very close if
not the same.
I poked at some 10 intel and AMD machines and the numbers are different
but the results seem more or less consistent with this.
From size point of view xchg is longer and xchgrz pokes at the red zone
which seems unnecessarily hacky, so good old lock+addl is probably the
best.
There isn't any extra magic behind mfence, is there?
E.g. I think lock orders accesses to WC memory as well,
so apparently mb() can be redefined unconditionally, without
looking at XMM2:
--->
x86: drop mfence in favor of lock+addl
mfence appears to be way slower than a locked instruction - let's use
lock+add unconditionally, same as we always did on old 32-bit.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I'll play with this some more before posting this as a
non-stand alone patch. Is there a macro-benchmark where mb
is prominent?
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a584e1c..f0d36e2 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -15,15 +15,15 @@
* Some non-Intel clones support out of order store. wmb() ceases to be a
* nop for these.
*/
-#define mb() alternative("lock; addl $0,0(%%esp)", "mfence", X86_FEATURE_XMM2)
+#define mb() asm volatile("lock; addl $0,0(%%esp)":::"memory")
#define rmb() alternative("lock; addl $0,0(%%esp)", "lfence", X86_FEATURE_XMM2)
#define wmb() alternative("lock; addl $0,0(%%esp)", "sfence", X86_FEATURE_XMM)
#else
+#define mb() asm volatile("lock; addl $0,0(%%rsp)":::"memory")
#define rmb() asm volatile("lfence":::"memory")
#define wmb() asm volatile("sfence" ::: "memory")
#endif
#ifdef CONFIG_X86_PPRO_FENCE
#define dma_rmb() rmb()
#else
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply related
* Re: [PATCH v3 00/41] arch: barrier cleanup + barriers for virt
From: Peter Zijlstra @ 2016-01-12 12:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, virtualization, H. Peter Anvin,
sparclinux, linux-arch, linux-s390, Russell King - ARM Linux,
Arnd Bergmann, x86, xen-devel, Ingo Molnar, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper,
linux-kernel, Joe Perches, linuxppc-dev, David Miller
In-Reply-To: <1452426622-4471-1-git-send-email-mst@redhat.com>
On Sun, Jan 10, 2016 at 04:16:22PM +0200, Michael S. Tsirkin wrote:
> I parked this in vhost tree for now, though the inclusion of patch 1 from tip
> creates a merge conflict - but one that is trivial to resolve.
>
> So I intend to just merge it all through my tree, including the
> duplicate patch, and assume conflict will be resolved.
>
> I would really appreciate some feedback on arch bits (especially the x86 bits),
> and acks for merging this through the vhost tree.
Thanks for doing this, looks good to me.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-12 11:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, user-mode-linux-devel, linux-sh,
Michael Ellerman, x86, xen-devel, Ingo Molnar, Paul McKenney,
linux-xtensa, james.hogan, Arnd Bergmann, Stefano Stabellini,
adi-buildroot-devel, Leonid Yegoshin, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <20160112104012.GW6373@twins.programming.kicks-ass.net>
On Tue, Jan 12, 2016 at 11:40:12AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 11:25:55AM +0100, Peter Zijlstra wrote:
> > On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
> > > 2) the changelog _completely_ fails to explain the sync 0x11 and sync
> > > 0x12 semantics nor does it provide a publicly accessible link to
> > > documentation that does.
> >
> > Ralf pointed me at: https://imgtec.com/mips/architectures/mips64/
> >
> > > 3) it really should have explained what you did with
> > > smp_llsc_mb/smp_mb__before_llsc() in _detail_.
> >
> > And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
> > are _NOT_ transitive and therefore cannot be used to implement the
> > smp_mb__{before,after} stuff.
> >
> > That is, in MIPS speak, those SYNC types are Ordering Barriers, not
> > Completion Barriers. They need not be globally performed.
>
> Which if true; and I know Will has some questions here; would also mean
> that you 'cannot' use the ACQUIRE/RELEASE barriers for your locks as was
> recently suggested by David Daney.
The issue I have with the SYNC description in the text above is that it
describes the single CPU (program order) and the dual-CPU (confusingly
named global order) cases, but then doesn't generalise any further. That
means we can't sensibly reason about transitivity properties when a third
agent is involved. For example, the WRC+sync+addr test:
P0:
Wx = 1
P1:
Rx == 1
SYNC
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
I can't find anything to forbid that, given the text. The main problem
is having the SYNC on P1 affect the write by P0.
> That is, currently all architectures -- with exception of PPC -- have
> RCsc locks, but using these non-transitive things will get you RCpc
> locks.
>
> So yes, MIPS can go RCpc for its locks and share the burden of pain with
> PPC, but that needs to be a very concious decision.
I think it's much worse than RCpc, given my interpretation of the wording.
Will
^ permalink raw reply
* Re: [PATCH] vhost: move is_le setup to the backend
From: Michael S. Tsirkin @ 2016-01-12 10:48 UTC (permalink / raw)
To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20160112113100.60e6b872@bahia.huguette.org>
On Tue, Jan 12, 2016 at 11:31:00AM +0100, Greg Kurz wrote:
> On Tue, 12 Jan 2016 12:01:32 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Jan 11, 2016 at 03:39:38PM +0100, Greg Kurz wrote:
> > > The vq->is_le field is used to fix endianness when accessing the vring via
> > > the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
> > >
> > > 1) host is big endian and device is modern virtio
> > >
> > > 2) host has cross-endian support and device is legacy virtio with a different
> > > endianness than the host
> > >
> > > Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
> > > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
> > > is only needed when the backend is active, it was decided to set it at
> > > backend start.
> > >
> > > This is currently done in vhost_init_used()->vhost_init_is_le() but it
> > > obfuscates the core vhost code. This patch moves the is_le setup to a
> > > dedicated function that is called from the backend code.
> > >
> > > Note vhost_net is the only backend that can pass vq->private_data == NULL to
> > > vhost_init_used(), hence the "if (sock)" branch.
> > >
> > > No behaviour change.
> > >
> > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > ---
> > >
> > > Hi Michael,
> > >
> > > This is a cleanup patch I had posted last year:
> > >
> > > http://patchwork.ozlabs.org/patch/538277/
> > >
> > > I am just reposting with Cornelia's R-b.
> > >
> > > Cheers.
> > >
> > > --
> > > Greg
> > >
> > > drivers/vhost/net.c | 6 ++++++
> > > drivers/vhost/scsi.c | 3 +++
> > > drivers/vhost/test.c | 2 ++
> > > drivers/vhost/vhost.c | 12 +++++++-----
> > > drivers/vhost/vhost.h | 1 +
> > > 5 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 9eda69e40678..d6319cb2664c 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> > >
> > > vhost_net_disable_vq(n, vq);
> > > vq->private_data = sock;
> > > +
> > > + if (sock)
> > > + vhost_set_is_le(vq);
> > > + else
> > > + vq->is_le = virtio_legacy_is_little_endian();
> > > +
> > > r = vhost_init_used(vq);
> > > if (r)
> > > goto err_used;
> >
> > This part is kind of ugly. I think it's cleaner is the generic code.
> > How about we teach vhost_set_is_le to test vq->private_data and DTRT?
> >
>
> You're right. I'll try that.
>
> > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > > index 29cfc57d496e..1f4f405ebba8 100644
> > > --- a/drivers/vhost/scsi.c
> > > +++ b/drivers/vhost/scsi.c
> > > @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> > > vq = &vs->vqs[i].vq;
> > > mutex_lock(&vq->mutex);
> > > vq->private_data = vs_tpg;
> > > +
> > > + vhost_set_is_le(vq);
> > > +
> > > vhost_init_used(vq);
> > > mutex_unlock(&vq->mutex);
> > > }
> > > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > > index f2882ac98726..b1c7df502211 100644
> > > --- a/drivers/vhost/test.c
> > > +++ b/drivers/vhost/test.c
> > > @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
> > > oldpriv = vq->private_data;
> > > vq->private_data = priv;
> > >
> > > + vhost_set_is_le(vq);
> > > +
> > > r = vhost_init_used(&n->vqs[index]);
> > >
> > > mutex_unlock(&vq->mutex);
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index ad2146a9ab2d..e688eb801d43 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > > }
> > > #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> > >
> > > +void vhost_set_is_le(struct vhost_virtqueue *vq)
> > > +{
> > > + vhost_init_is_le(vq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(vhost_set_is_le);
> > > +
> > > static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> > > poll_table *pt)
> > > {
> >
> > Why do we need this wrapper? Why not just export vhost_init_is_le?
>
> This is a tentative workaround to:
>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #35: FILE: drivers/vhost/vhost.c:116:
> +EXPORT_SYMBOL_GPL(vhost_init_is_le);
>
> Other possibility is to put the EXPORT_SYMBOL_GPL statement in both
> conditional compilation blocks. Maybe better ?
That, or ignore the warning.
> > Or rename vhost_init_is_le->vhost_set_is_le if you prefer.
> >
> > > @@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> > > {
> > > __virtio16 last_used_idx;
> > > int r;
> > > - if (!vq->private_data) {
> > > - vq->is_le = virtio_legacy_is_little_endian();
> > > + if (!vq->private_data)
> > > return 0;
> > > - }
> > > -
> > > - vhost_init_is_le(vq);
> > >
> > > r = vhost_update_used_flags(vq);
> > > if (r)
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index d3f767448a72..af5d33797937 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> > >
> > > int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> > > unsigned int log_num, u64 len);
> > > +void vhost_set_is_le(struct vhost_virtqueue *vq);
> > >
> > > #define vq_err(vq, fmt, ...) do { \
> > > pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> >
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-12 10:40 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, will.deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux, Arnd Bergmann,
linux-sh, Michael Ellerman, x86, xen-devel, Ingo Molnar,
Paul McKenney, linux-xtensa, james.hogan, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <20160112102555.GV6373@twins.programming.kicks-ass.net>
On Tue, Jan 12, 2016 at 11:25:55AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
> > 2) the changelog _completely_ fails to explain the sync 0x11 and sync
> > 0x12 semantics nor does it provide a publicly accessible link to
> > documentation that does.
>
> Ralf pointed me at: https://imgtec.com/mips/architectures/mips64/
>
> > 3) it really should have explained what you did with
> > smp_llsc_mb/smp_mb__before_llsc() in _detail_.
>
> And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
> are _NOT_ transitive and therefore cannot be used to implement the
> smp_mb__{before,after} stuff.
>
> That is, in MIPS speak, those SYNC types are Ordering Barriers, not
> Completion Barriers. They need not be globally performed.
Which if true; and I know Will has some questions here; would also mean
that you 'cannot' use the ACQUIRE/RELEASE barriers for your locks as was
recently suggested by David Daney.
That is, currently all architectures -- with exception of PPC -- have
RCsc locks, but using these non-transitive things will get you RCpc
locks.
So yes, MIPS can go RCpc for its locks and share the burden of pain with
PPC, but that needs to be a very concious decision.
^ permalink raw reply
* Re: [PATCH] vhost: move is_le setup to the backend
From: Greg Kurz @ 2016-01-12 10:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20160112115639-mutt-send-email-mst@redhat.com>
On Tue, 12 Jan 2016 12:01:32 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jan 11, 2016 at 03:39:38PM +0100, Greg Kurz wrote:
> > The vq->is_le field is used to fix endianness when accessing the vring via
> > the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
> >
> > 1) host is big endian and device is modern virtio
> >
> > 2) host has cross-endian support and device is legacy virtio with a different
> > endianness than the host
> >
> > Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
> > VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
> > is only needed when the backend is active, it was decided to set it at
> > backend start.
> >
> > This is currently done in vhost_init_used()->vhost_init_is_le() but it
> > obfuscates the core vhost code. This patch moves the is_le setup to a
> > dedicated function that is called from the backend code.
> >
> > Note vhost_net is the only backend that can pass vq->private_data == NULL to
> > vhost_init_used(), hence the "if (sock)" branch.
> >
> > No behaviour change.
> >
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >
> > Hi Michael,
> >
> > This is a cleanup patch I had posted last year:
> >
> > http://patchwork.ozlabs.org/patch/538277/
> >
> > I am just reposting with Cornelia's R-b.
> >
> > Cheers.
> >
> > --
> > Greg
> >
> > drivers/vhost/net.c | 6 ++++++
> > drivers/vhost/scsi.c | 3 +++
> > drivers/vhost/test.c | 2 ++
> > drivers/vhost/vhost.c | 12 +++++++-----
> > drivers/vhost/vhost.h | 1 +
> > 5 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 9eda69e40678..d6319cb2664c 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> >
> > vhost_net_disable_vq(n, vq);
> > vq->private_data = sock;
> > +
> > + if (sock)
> > + vhost_set_is_le(vq);
> > + else
> > + vq->is_le = virtio_legacy_is_little_endian();
> > +
> > r = vhost_init_used(vq);
> > if (r)
> > goto err_used;
>
> This part is kind of ugly. I think it's cleaner is the generic code.
> How about we teach vhost_set_is_le to test vq->private_data and DTRT?
>
You're right. I'll try that.
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 29cfc57d496e..1f4f405ebba8 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> > vq = &vs->vqs[i].vq;
> > mutex_lock(&vq->mutex);
> > vq->private_data = vs_tpg;
> > +
> > + vhost_set_is_le(vq);
> > +
> > vhost_init_used(vq);
> > mutex_unlock(&vq->mutex);
> > }
> > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index f2882ac98726..b1c7df502211 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
> > oldpriv = vq->private_data;
> > vq->private_data = priv;
> >
> > + vhost_set_is_le(vq);
> > +
> > r = vhost_init_used(&n->vqs[index]);
> >
> > mutex_unlock(&vq->mutex);
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index ad2146a9ab2d..e688eb801d43 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> > }
> > #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> >
> > +void vhost_set_is_le(struct vhost_virtqueue *vq)
> > +{
> > + vhost_init_is_le(vq);
> > +}
> > +EXPORT_SYMBOL_GPL(vhost_set_is_le);
> > +
> > static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> > poll_table *pt)
> > {
>
> Why do we need this wrapper? Why not just export vhost_init_is_le?
This is a tentative workaround to:
WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#35: FILE: drivers/vhost/vhost.c:116:
+EXPORT_SYMBOL_GPL(vhost_init_is_le);
Other possibility is to put the EXPORT_SYMBOL_GPL statement in both
conditional compilation blocks. Maybe better ?
> Or rename vhost_init_is_le->vhost_set_is_le if you prefer.
>
> > @@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> > {
> > __virtio16 last_used_idx;
> > int r;
> > - if (!vq->private_data) {
> > - vq->is_le = virtio_legacy_is_little_endian();
> > + if (!vq->private_data)
> > return 0;
> > - }
> > -
> > - vhost_init_is_le(vq);
> >
> > r = vhost_update_used_flags(vq);
> > if (r)
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index d3f767448a72..af5d33797937 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> >
> > int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> > unsigned int log_num, u64 len);
> > +void vhost_set_is_le(struct vhost_virtqueue *vq);
> >
> > #define vq_err(vq, fmt, ...) do { \
> > pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
>
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-12 10:25 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, will.deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux, Arnd Bergmann,
linux-sh, x86, xen-devel, Ingo Molnar, linux-xtensa, james.hogan,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper, linux-kernel
In-Reply-To: <20160112092711.GP6344@twins.programming.kicks-ass.net>
On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
> 2) the changelog _completely_ fails to explain the sync 0x11 and sync
> 0x12 semantics nor does it provide a publicly accessible link to
> documentation that does.
Ralf pointed me at: https://imgtec.com/mips/architectures/mips64/
> 3) it really should have explained what you did with
> smp_llsc_mb/smp_mb__before_llsc() in _detail_.
And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
are _NOT_ transitive and therefore cannot be used to implement the
smp_mb__{before,after} stuff.
That is, in MIPS speak, those SYNC types are Ordering Barriers, not
Completion Barriers. They need not be globally performed.
^ permalink raw reply
* Re: [PATCH] vhost: move is_le setup to the backend
From: Michael S. Tsirkin @ 2016-01-12 10:01 UTC (permalink / raw)
To: Greg Kurz; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20160111143509.9601.15143.stgit@bahia.huguette.org>
On Mon, Jan 11, 2016 at 03:39:38PM +0100, Greg Kurz wrote:
> The vq->is_le field is used to fix endianness when accessing the vring via
> the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
>
> 1) host is big endian and device is modern virtio
>
> 2) host has cross-endian support and device is legacy virtio with a different
> endianness than the host
>
> Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
> VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
> is only needed when the backend is active, it was decided to set it at
> backend start.
>
> This is currently done in vhost_init_used()->vhost_init_is_le() but it
> obfuscates the core vhost code. This patch moves the is_le setup to a
> dedicated function that is called from the backend code.
>
> Note vhost_net is the only backend that can pass vq->private_data == NULL to
> vhost_init_used(), hence the "if (sock)" branch.
>
> No behaviour change.
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>
> Hi Michael,
>
> This is a cleanup patch I had posted last year:
>
> http://patchwork.ozlabs.org/patch/538277/
>
> I am just reposting with Cornelia's R-b.
>
> Cheers.
>
> --
> Greg
>
> drivers/vhost/net.c | 6 ++++++
> drivers/vhost/scsi.c | 3 +++
> drivers/vhost/test.c | 2 ++
> drivers/vhost/vhost.c | 12 +++++++-----
> drivers/vhost/vhost.h | 1 +
> 5 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9eda69e40678..d6319cb2664c 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>
> vhost_net_disable_vq(n, vq);
> vq->private_data = sock;
> +
> + if (sock)
> + vhost_set_is_le(vq);
> + else
> + vq->is_le = virtio_legacy_is_little_endian();
> +
> r = vhost_init_used(vq);
> if (r)
> goto err_used;
This part is kind of ugly. I think it's cleaner is the generic code.
How about we teach vhost_set_is_le to test vq->private_data and DTRT?
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 29cfc57d496e..1f4f405ebba8 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> vq = &vs->vqs[i].vq;
> mutex_lock(&vq->mutex);
> vq->private_data = vs_tpg;
> +
> + vhost_set_is_le(vq);
> +
> vhost_init_used(vq);
> mutex_unlock(&vq->mutex);
> }
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index f2882ac98726..b1c7df502211 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
> oldpriv = vq->private_data;
> vq->private_data = priv;
>
> + vhost_set_is_le(vq);
> +
> r = vhost_init_used(&n->vqs[index]);
>
> mutex_unlock(&vq->mutex);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index ad2146a9ab2d..e688eb801d43 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
> }
> #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
>
> +void vhost_set_is_le(struct vhost_virtqueue *vq)
> +{
> + vhost_init_is_le(vq);
> +}
> +EXPORT_SYMBOL_GPL(vhost_set_is_le);
> +
> static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> poll_table *pt)
> {
Why do we need this wrapper? Why not just export vhost_init_is_le?
Or rename vhost_init_is_le->vhost_set_is_le if you prefer.
> @@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> {
> __virtio16 last_used_idx;
> int r;
> - if (!vq->private_data) {
> - vq->is_le = virtio_legacy_is_little_endian();
> + if (!vq->private_data)
> return 0;
> - }
> -
> - vhost_init_is_le(vq);
>
> r = vhost_update_used_flags(vq);
> if (r)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d3f767448a72..af5d33797937 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> unsigned int log_num, u64 len);
> +void vhost_set_is_le(struct vhost_virtqueue *vq);
>
> #define vq_err(vq, fmt, ...) do { \
> pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-12 9:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, virtualization, H. Peter Anvin,
sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
Thomas Gleixner, linux-metag, linux-arm-kernel, Andrew Cooper,
linux-kernel, Ralf Baechle
In-Reply-To: <20160112103913-mutt-send-email-mst@redhat.com>
On Tue, Jan 12, 2016 at 10:43:36AM +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
> > On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
> > >On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
> > >smp_read_barrier_depends, smp_store_release and smp_load_acquire match
> > >the asm-generic variants exactly. Drop the local definitions and pull in
> > >asm-generic/barrier.h instead.
> > >
> > This statement doesn't fit MIPS barriers variations. Moreover, there is a
> > reason to extend that even more specific, at least for smp_store_release and
> > smp_load_acquire, look into
> >
> > http://patchwork.linux-mips.org/patch/10506/
> >
> > - Leonid.
>
> Fine, but it matches what current code is doing. Since that
> MIPS_LIGHTWEIGHT_SYNC patch didn't go into linux-next yet, do
> you see a problem reworking it on top of this patchset?
That patch is a complete doorstop atm. It needs a lot more work before
it can go anywhere. Don't worry about it.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-12 9:27 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, will.deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux, Arnd Bergmann,
linux-sh, x86, xen-devel, Ingo Molnar, linux-xtensa, james.hogan,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper, linux-kernel
In-Reply-To: <56945366.2090504@imgtec.com>
On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
> This statement doesn't fit MIPS barriers variations. Moreover, there is a
> reason to extend that even more specific, at least for smp_store_release and
> smp_load_acquire, look into
>
> http://patchwork.linux-mips.org/patch/10506/
Dude, that's one horrible patch.
1) you do not make such things selectable; either the hardware needs
them or it doesn't. If it does you _must_ use them, however unlikely.
2) the changelog _completely_ fails to explain the sync 0x11 and sync
0x12 semantics nor does it provide a publicly accessible link to
documentation that does.
3) it really should have explained what you did with
smp_llsc_mb/smp_mb__before_llsc() in _detail_.
And I agree that ideally it should be split into parts.
Seriously, this is _NOT_ OK.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-12 8:43 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, linux-kernel,
Ralf Baechle, Joe Perches
In-Reply-To: <56945366.2090504@imgtec.com>
On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
> On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
> >On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
> >smp_read_barrier_depends, smp_store_release and smp_load_acquire match
> >the asm-generic variants exactly. Drop the local definitions and pull in
> >asm-generic/barrier.h instead.
> >
> This statement doesn't fit MIPS barriers variations. Moreover, there is a
> reason to extend that even more specific, at least for smp_store_release and
> smp_load_acquire, look into
>
> http://patchwork.linux-mips.org/patch/10506/
>
> - Leonid.
Fine, but it matches what current code is doing. Since that
MIPS_LIGHTWEIGHT_SYNC patch didn't go into linux-next yet, do
you see a problem reworking it on top of this patchset?
--
MST
^ permalink raw reply
* 15-day Public Review for Virtual I/O Device (VIRTIO) Version 1.0 - ends January 26th
From: Chet Ensign @ 2016-01-12 1:28 UTC (permalink / raw)
To: tc-announce, members, virtio, virtio-comment
[-- Attachment #1.1: Type: text/plain, Size: 4206 bytes --]
OASIS members and other interested parties,
The OASIS Virtual I/O Device (VIRTIO) TC members [1] have produced an
updated Committee Specification Draft (CSD) and submitted this
specification for 15-day public review:
Virtual I/O Device (VIRTIO) Version 1.0
Committee Specification Draft 05 / Public Review Draft 05
29 October 2015
Specification Overview:
This document describes the specifications of the "virtio" family of
devices. These devices are found in virtual environments, yet by design
they are not all that different from physical devices, and this document
treats them as such. This allows the guest to use standard drivers and
discovery mechanisms.
The purpose of virtio and this specification is that virtual environments
and guests should have a straightforward, efficient, standard and
extensible mechanism for virtual devices, rather than boutique
per-environment or per-OS mechanisms.
Public Review Period:
The public review starts 12 January 2016 at 00:00 UTC and ends 26 January
2016 at 11:59 UTC. This public review draft makes changes to Committee
Specification 03. The changes are tracking in the red-line file at
http://docs.oasis-open.org/virtio/virtio/v1.0/cs03/virtio-v1.0-cs03-diff.html.
This is an open invitation to comment. OASIS solicits feedback from
potential users, developers and others, whether OASIS members or not, for
the sake of improving the interoperability and quality of its technical
work.
URIs:
The prose specification document and related files are available here:
Editable source (Authoritative):
http://docs.oasis-open.org/virtio/virtio/v1.0/csprd05/tex/
HTML:
http://docs.oasis-open.org/virtio/virtio/v1.0/csprd05/virtio-v1.0-csprd05.html
PDF:
http://docs.oasis-open.org/virtio/virtio/v1.0/csprd05/virtio-v1.0-csprd05.pdf
Example Driver Listing:
http://docs.oasis-open.org/virtio/virtio/v1.0/csprd05/listings/
ZIP distribution files (complete):
For your convenience, OASIS provides a complete package of the prose
specification and related files in a ZIP distribution file. You can
download the ZIP file here:
http://docs.oasis-open.org/virtio/virtio/v1.0/csprd05/virtio-v1.0-csprd05.zip
Additional information about this specification and the VIRTIO TC may be
found on the TC's public home page located at:
http://www.oasis-open.org/committees/virtio/
Comments may be submitted to the TC by any person through the use of the
OASIS TC Comment Facility which can be accessed via the button labeled
"Send A Comment" at the top of the TC public home page, or directly at:
http://www.oasis-open.org/committees/comments/form.php?wg_abbrev=virtio
Feedback submitted by TC non-members for this work and for other work of
this TC is publicly archived and can be viewed at:
http://lists.oasis-open.org/archives/virtio-comment/
All comments submitted to OASIS are subject to the OASIS Feedback License,
which ensures that the feedback you provide carries the same obligations at
least as the obligations of the TC members. In connection with this public
review of 'Virtual I/O Device (VIRTIO) Version 1.0', we call your attention
to the OASIS IPR Policy [2] applicable especially [3] to the work of this
technical committee. All members of the TC should be familiar with this
document, which may create obligations regarding the disclosure and
availability of a member's patent, copyright, trademark and license rights
that read on an approved OASIS specification.
OASIS invites any persons who know of any such claims to disclose these if
they may be essential to the implementation of the above specification, so
that notice of them may be posted to the notice page for this TC's work.
========== Additional references:
[1] OASIS Virtual I/O Device (VIRTIO) TC
http://www.oasis-open.org/committees/virtio/
[2] http://www.oasis-open.org/policies-guidelines/ipr
[3] http://www.oasis-open.org/committees/virtio/ipr.php
https://www.oasis-open.org/policies-guidelines/ipr#licensing_req
Non-Assertion Mode
--
/chet
----------------
Chet Ensign
Director of Standards Development and TC Administration
OASIS: Advancing open standards for the information society
http://www.oasis-open.org
Primary: +1 973-996-2298
Mobile: +1 201-341-1393
[-- Attachment #1.2: Type: text/html, Size: 6267 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-12 1:14 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, xen-devel,
Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, Ralf Baechle,
Joe Perches, linuxppc-dev
In-Reply-To: <1452426622-4471-12-git-send-email-mst@redhat.com>
On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
> On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
> smp_read_barrier_depends, smp_store_release and smp_load_acquire match
> the asm-generic variants exactly. Drop the local definitions and pull in
> asm-generic/barrier.h instead.
>
This statement doesn't fit MIPS barriers variations. Moreover, there is
a reason to extend that even more specific, at least for
smp_store_release and smp_load_acquire, look into
http://patchwork.linux-mips.org/patch/10506/
- Leonid.
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC 0/3] Xen on Virtio
From: Andy Lutomirski @ 2016-01-11 23:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Stabellini, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, Linux Virtualization, David Vrabel,
xen-devel@lists.xenproject.org
In-Reply-To: <20151215204024.GA26499@redhat.com>
On Tue, Dec 15, 2015 at 12:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Dec 14, 2015 at 10:27:52AM -0800, Andy Lutomirski wrote:
>> On Mon, Dec 14, 2015 at 6:12 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Dec 14, 2015 at 02:00:05PM +0000, David Vrabel wrote:
>> >> On 07/12/15 16:19, Stefano Stabellini wrote:
>> >> > Hi all,
>> >> >
>> >> > this patch series introduces support for running Linux on top of Xen
>> >> > inside a virtual machine with virtio devices (nested virt scenario).
>> >> > The problem is that Linux virtio drivers use virt_to_phys to get the
>> >> > guest pseudo-physical addresses to pass to the backend, which doesn't
>> >> > work as expected on Xen.
>> >> >
>> >> > Switching the virtio drivers to the dma APIs (dma_alloc_coherent,
>> >> > dma_map/unmap_single and dma_map/unmap_sg) would solve the problem, as
>> >> > Xen support in Linux provides an implementation of the dma API which
>> >> > takes care of the additional address conversions. However using the dma
>> >> > API would increase the complexity of the non-Xen case too. We would also
>> >> > need to keep track of the physical or virtual address in addition to the
>> >> > dma address for each vring_desc to be able to free the memory in
>> >> > detach_buf (see patch #3).
>> >> >
>> >> > Instead this series adds few obvious checks to perform address
>> >> > translations in a couple of key places, without changing non-Xen code
>> >> > paths. You are welcome to suggest improvements or alternative
>> >> > implementations.
>> >>
>> >> Andy Lutomirski also looked at this. Andy what happened to this work?
>> >>
>> >> David
>> >
>> > The approach there was to try and convert all virtio to use DMA
>> > API unconditionally.
>> > This is reasonable if there's a way for devices to request
>> > 1:1 mappings individually.
>> > As that is currently missing, that patchset can not be merged yet.
>> >
>>
>> I still don't understand why *devices* need the ability to request
>> anything in particular.
>
> See below.
>
>> In current kernels, devices that don't have
>> an iommu work (and there's no choice about 1:1 or otherwise) and
>> devices that have an iommu fail spectacularly. With the patches,
>> devices that don't have an iommu continue to work as long as the DMA
>> API and/or virtio correctly knows that there's no iommu. Devices that
>> do have an iommu work fine, albeit slower than would be ideal. In my
>> book, slower than would be ideal is strictly better than crashing.
>>
>> The real issue is *detecting* whether there's an iommu, and the string
>> of bugs in that area (buggy QEMU for the Q35 thing and complete lack
>> of a solution for PPC and SPARC is indeed a problem).
>>
>> I think that we could apply the series ending here:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=virtio_dma&id=ad9d43052da44ce18363c02ea597dde01eeee11b
>>
>> and the only regression (performance or functionality) would be that
>> the buggy Q35 iommu configuration would stop working until someone
>> fixed it in QEMU. That should be okay -- it's explicitly
>> experimental. (Xen works with that series applied.) (Actually,
>> there might be a slight performance regression on PPC due to extra
>> unused mappings being created. It would be straightforward to hack
>> around that in one of several ways.)
>>
>> Am I missing something?
>>
>> --Andy
>
> I think there's more to virtio than just QEMU.
>
> I have no idea whether anyone implemented hypervisors with an IOMMU.
> virtio bypassing iommu makes a lot of sense so it did this since
> forever. I do not feel comfortable changing guest/hypervisor ABI and
> waiting for people to complain.
>
> But we do want to fix Xen.
>
> Let's do this slowly, and whitelist the configurations that
> require DMA API to work, so we know we are not breaking anything.
>
> For example, test a device flag and use iommu if set.
> Currently, set it if xen_pv_domain is enabled.
> We'll add more as more platforms gain IOMMU support
> for virtio and we find ways to identify them.
>
> It would be kind of a mix of what you did and what Stefano did.
>
> And alternative would be a quirk: make DMA API create 1:1 mappings for
> virtio devices only. Then teach Xen pv to ignore this quirk. This is
> what I referred to above.
> For example, something like DMA_ATTR_IOMMU_BYPASS would do the trick
> nicely. If there's a chance that's going to be upstream, we
> could use that.
I'd be in favor of that approach, except that apparently PowerPC can't
do it (the 1:1 mappings have an offset). I *think* that x86 can do
it.
I'll re-send the series with DMA API defaulted off except on Xen once
the merge window closes.
--Andy
^ permalink raw reply
* Re: [PATCH v4 0/3] checkpatch: handling of memory barriers
From: Joe Perches @ 2016-01-11 21:45 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
In-Reply-To: <1452509968-19778-1-git-send-email-mst@redhat.com>
On Mon, 2016-01-11 at 13:00 +0200, Michael S. Tsirkin wrote:
> As part of memory barrier cleanup, this patchset
> extends checkpatch to make it easier to stop
> incorrect memory barrier usage.
Thanks Michael.
Acked-by: Joe Perches <joe@perches.com>
^ permalink raw reply
* [PATCH] vhost: move is_le setup to the backend
From: Greg Kurz @ 2016-01-11 14:39 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
The vq->is_le field is used to fix endianness when accessing the vring via
the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
1) host is big endian and device is modern virtio
2) host has cross-endian support and device is legacy virtio with a different
endianness than the host
Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
is only needed when the backend is active, it was decided to set it at
backend start.
This is currently done in vhost_init_used()->vhost_init_is_le() but it
obfuscates the core vhost code. This patch moves the is_le setup to a
dedicated function that is called from the backend code.
Note vhost_net is the only backend that can pass vq->private_data == NULL to
vhost_init_used(), hence the "if (sock)" branch.
No behaviour change.
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
Hi Michael,
This is a cleanup patch I had posted last year:
http://patchwork.ozlabs.org/patch/538277/
I am just reposting with Cornelia's R-b.
Cheers.
--
Greg
drivers/vhost/net.c | 6 ++++++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 12 +++++++-----
drivers/vhost/vhost.h | 1 +
5 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e40678..d6319cb2664c 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
vhost_net_disable_vq(n, vq);
vq->private_data = sock;
+
+ if (sock)
+ vhost_set_is_le(vq);
+ else
+ vq->is_le = virtio_legacy_is_little_endian();
+
r = vhost_init_used(vq);
if (r)
goto err_used;
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 29cfc57d496e..1f4f405ebba8 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
vq = &vs->vqs[i].vq;
mutex_lock(&vq->mutex);
vq->private_data = vs_tpg;
+
+ vhost_set_is_le(vq);
+
vhost_init_used(vq);
mutex_unlock(&vq->mutex);
}
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index f2882ac98726..b1c7df502211 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
oldpriv = vq->private_data;
vq->private_data = priv;
+ vhost_set_is_le(vq);
+
r = vhost_init_used(&n->vqs[index]);
mutex_unlock(&vq->mutex);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ad2146a9ab2d..e688eb801d43 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
}
#endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
+void vhost_set_is_le(struct vhost_virtqueue *vq)
+{
+ vhost_init_is_le(vq);
+}
+EXPORT_SYMBOL_GPL(vhost_set_is_le);
+
static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
poll_table *pt)
{
@@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
{
__virtio16 last_used_idx;
int r;
- if (!vq->private_data) {
- vq->is_le = virtio_legacy_is_little_endian();
+ if (!vq->private_data)
return 0;
- }
-
- vhost_init_is_le(vq);
r = vhost_update_used_flags(vq);
if (r)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d3f767448a72..af5d33797937 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
+void vhost_set_is_le(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
^ permalink raw reply related
* Re: [PATCH v3 39/41] xen/events: use virt_xxx barriers
From: David Vrabel @ 2016-01-11 11:12 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
Stefano Stabellini, H. Peter Anvin, sparclinux, Boris Ostrovsky,
linux-arch, linux-s390, Russell King - ARM Linux, Arnd Bergmann,
x86, xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Wei Liu,
Konrad Rzeszutek Wilk <konrad.wil>
In-Reply-To: <1452426622-4471-40-git-send-email-mst@redhat.com>
On 10/01/16 14:21, Michael S. Tsirkin wrote:
> drivers/xen/events/events_fifo.c uses rmb() to communicate with the
> other side.
>
> For guests compiled with CONFIG_SMP, smp_rmb would be sufficient, so
> rmb() here is only needed if a non-SMP guest runs on an SMP host.
>
> Switch to the virt_rmb barrier which serves this exact purpose.
>
> Pull in asm/barrier.h here to make sure the file is self-contained.
>
> Suggested-by: David Vrabel <david.vrabel@citrix.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
David
^ permalink raw reply
* Re: [PATCH v2 20/32] metag: define __smp_xxx
From: Michael S. Tsirkin @ 2016-01-11 11:10 UTC (permalink / raw)
To: James Hogan
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Arnd Bergmann, Davidlohr Bueso, x86, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel, Stefano Stabellini,
Andrey Konovalov, adi-buildroot-devel, Thomas Gleixner,
linux-metag, linux-arm-kernel, Andrew Cooper, linux-kernel,
linuxppc-dev
In-Reply-To: <20160105000929.GM17861@jhogan-linux.le.imgtec.org>
On Tue, Jan 05, 2016 at 12:09:30AM +0000, James Hogan wrote:
> Hi Michael,
>
> On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
> > This defines __smp_xxx barriers for metag,
> > for use by virtualization.
> >
> > smp_xxx barriers are removed as they are
> > defined correctly by asm-generic/barriers.h
> >
> > Note: as __smp_XX macros should not depend on CONFIG_SMP, they can not
> > use the existing fence() macro since that is defined differently between
> > SMP and !SMP. For this reason, this patch introduces a wrapper
> > metag_fence() that doesn't depend on CONFIG_SMP.
> > fence() is then defined using that, depending on CONFIG_SMP.
>
> I'm not a fan of the inconsistent commit message wrapping. I wrap to 72
> columns (although I now notice SubmittingPatches says to use 75...).
>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > arch/metag/include/asm/barrier.h | 32 +++++++++++++++-----------------
> > 1 file changed, 15 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/metag/include/asm/barrier.h b/arch/metag/include/asm/barrier.h
> > index b5b778b..84880c9 100644
> > --- a/arch/metag/include/asm/barrier.h
> > +++ b/arch/metag/include/asm/barrier.h
> > @@ -44,13 +44,6 @@ static inline void wr_fence(void)
> > #define rmb() barrier()
> > #define wmb() mb()
> >
> > -#ifndef CONFIG_SMP
> > -#define fence() do { } while (0)
> > -#define smp_mb() barrier()
> > -#define smp_rmb() barrier()
> > -#define smp_wmb() barrier()
> > -#else
>
> !SMP kernel text differs, but only because of new presence of unused
> metag_fence() inline function. If I #if 0 that out, then it matches, so
> thats fine.
>
> > -
> > #ifdef CONFIG_METAG_SMP_WRITE_REORDERING
> > /*
> > * Write to the atomic memory unlock system event register (command 0). This is
> > @@ -60,26 +53,31 @@ static inline void wr_fence(void)
> > * incoherence). It is therefore ineffective if used after and on the same
> > * thread as a write.
> > */
> > -static inline void fence(void)
> > +static inline void metag_fence(void)
> > {
> > volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK;
> > barrier();
> > *flushptr = 0;
> > barrier();
> > }
> > -#define smp_mb() fence()
> > -#define smp_rmb() fence()
> > -#define smp_wmb() barrier()
> > +#define __smp_mb() metag_fence()
> > +#define __smp_rmb() metag_fence()
> > +#define __smp_wmb() barrier()
> > #else
> > -#define fence() do { } while (0)
> > -#define smp_mb() barrier()
> > -#define smp_rmb() barrier()
> > -#define smp_wmb() barrier()
> > +#define metag_fence() do { } while (0)
> > +#define __smp_mb() barrier()
> > +#define __smp_rmb() barrier()
> > +#define __smp_wmb() barrier()
>
> Whitespace is now messed up. Admitedly its already inconsistent
> tabs/spaces, but it'd be nice if the definitions at least still all
> lined up. You're touching all the definitions which use spaces anyway,
> so feel free to convert them to tabs while you're at it.
>
> Other than those niggles, it looks sensible to me:
> Acked-by: James Hogan <james.hogan@imgtec.com>
>
> Cheers
> James
Thanks!
I did this in my tree (replaced spaces with tabs in the new
definitions); not reposting just because of this change.
> > #endif
> > +
> > +#ifdef CONFIG_SMP
> > +#define fence() metag_fence()
> > +#else
> > +#define fence() do { } while (0)
> > #endif
> >
> > -#define smp_mb__before_atomic() barrier()
> > -#define smp_mb__after_atomic() barrier()
> > +#define __smp_mb__before_atomic() barrier()
> > +#define __smp_mb__after_atomic() barrier()
> >
> > #include <asm-generic/barrier.h>
> >
> > --
> > MST
> >
^ permalink raw reply
* Re: [PATCH v4 0/3] checkpatch: handling of memory barriers
From: Julian Calaby @ 2016-01-11 11:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, Andy Whitcroft,
Thomas Gleixner, linux-metag, Mailing List, Arm,
Andrew Cooper <andrew.co>
In-Reply-To: <20160111130334-mutt-send-email-mst@redhat.com>
Hi Michael,
On Mon, Jan 11, 2016 at 10:04 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 11, 2016 at 12:59:25PM +0200, Michael S. Tsirkin wrote:
>> As part of memory barrier cleanup, this patchset
>> extends checkpatch to make it easier to stop
>> incorrect memory barrier usage.
>>
>> This replaces the checkpatch patches in my series
>> arch: barrier cleanup + barriers for virt
>> and will be included in the pull request including
>> the series.
>>
>> changes from v3:
>> rename smp_barrier_stems to barrier_stems
>> as suggested by Julian Calaby.
>
> In fact it was Joe Perches that suggested it.
> Sorry about the confusion.
I was about to point that out.
FWIW this entire series is:
Acked-by: Julian Calaby <julian.calaby@gmail.com>
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply
* Re: [PATCH v4 0/3] checkpatch: handling of memory barriers
From: Michael S. Tsirkin @ 2016-01-11 11:04 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
In-Reply-To: <1452509968-19778-1-git-send-email-mst@redhat.com>
On Mon, Jan 11, 2016 at 12:59:25PM +0200, Michael S. Tsirkin wrote:
> As part of memory barrier cleanup, this patchset
> extends checkpatch to make it easier to stop
> incorrect memory barrier usage.
>
> This replaces the checkpatch patches in my series
> arch: barrier cleanup + barriers for virt
> and will be included in the pull request including
> the series.
>
> changes from v3:
> rename smp_barrier_stems to barrier_stems
> as suggested by Julian Calaby.
In fact it was Joe Perches that suggested it.
Sorry about the confusion.
> add (?: ... ) around a variable in regexp,
> in case we change the value later so that it matters.
> changes from v2:
> address comments by Joe Perches:
> use (?: ... ) to avoid unnecessary capture groups
> rename smp_barriers to smp_barrier_stems for clarity
> add barriers before/after atomic
> Changes from v1:
> catch optional\s* before () in barriers
> rewrite using qr{} instead of map
>
> Michael S. Tsirkin (3):
> checkpatch.pl: add missing memory barriers
> checkpatch: check for __smp outside barrier.h
> checkpatch: add virt barriers
>
> Michael S. Tsirkin (3):
> checkpatch.pl: add missing memory barriers
> checkpatch: check for __smp outside barrier.h
> checkpatch: add virt barriers
>
> scripts/checkpatch.pl | 33 ++++++++++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> --
> MST
^ permalink raw reply
* [PATCH v4 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-11 11:00 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
In-Reply-To: <1452509968-19778-1-git-send-email-mst@redhat.com>
Add virt_ barriers to list of barriers to check for
presence of a comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25476c2..c7bf1aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5133,7 +5133,8 @@ sub process {
}x;
my $all_barriers = qr{
(?:$barriers)|
- smp_(?:$barrier_stems)
+ smp_(?:$barrier_stems)|
+ virt_(?:$barrier_stems)
}x;
if ($line =~ /\b(?:$all_barriers)\s*\(/) {
--
MST
^ permalink raw reply related
* [PATCH v4 2/3] checkpatch: check for __smp outside barrier.h
From: Michael S. Tsirkin @ 2016-01-11 11:00 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
In-Reply-To: <1452509968-19778-1-git-send-email-mst@redhat.com>
Introduction of __smp barriers cleans up a bunch of duplicate code, but
it gives people an additional handle onto a "new" set of barriers - just
because they're prefixed with __* unfortunately doesn't stop anyone from
using it (as happened with other arch stuff before.)
Add a checkpatch test so it will trigger a warning.
Reported-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 94b4e33..25476c2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5143,6 +5143,16 @@ sub process {
}
}
+ my $underscore_smp_barriers = qr{__smp_(?:$barrier_stems)}x;
+
+ if ($realfile !~ m@^include/asm-generic/@ &&
+ $realfile !~ m@/barrier\.h$@ &&
+ $line =~ m/\b(?:$underscore_smp_barriers)\s*\(/ &&
+ $line !~ m/^.\s*\#\s*define\s+(?:$underscore_smp_barriers)\s*\(/) {
+ WARN("MEMORY_BARRIER",
+ "__smp memory barriers shouldn't be used outside barrier.h and asm-generic\n" . $herecurr);
+ }
+
# check for waitqueue_active without a comment.
if ($line =~ /\bwaitqueue_active\s*\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
--
MST
^ permalink raw reply related
* [PATCH v4 1/3] checkpatch.pl: add missing memory barriers
From: Michael S. Tsirkin @ 2016-01-11 11:00 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
In-Reply-To: <1452509968-19778-1-git-send-email-mst@redhat.com>
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b3c228..94b4e33 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5116,7 +5116,27 @@ sub process {
}
}
# check for memory barriers without a comment.
- if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
+
+ my $barriers = qr{
+ mb|
+ rmb|
+ wmb|
+ read_barrier_depends
+ }x;
+ my $barrier_stems = qr{
+ mb__before_atomic|
+ mb__after_atomic|
+ store_release|
+ load_acquire|
+ store_mb|
+ (?:$barriers)
+ }x;
+ my $all_barriers = qr{
+ (?:$barriers)|
+ smp_(?:$barrier_stems)
+ }x;
+
+ if ($line =~ /\b(?:$all_barriers)\s*\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
"memory barrier without comment\n" . $herecurr);
--
MST
^ permalink raw reply related
* [PATCH v4 0/3] checkpatch: handling of memory barriers
From: Michael S. Tsirkin @ 2016-01-11 11:00 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Julian Calaby, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, linux-arm-kernel,
Andrew Cooper <and>
As part of memory barrier cleanup, this patchset
extends checkpatch to make it easier to stop
incorrect memory barrier usage.
This replaces the checkpatch patches in my series
arch: barrier cleanup + barriers for virt
and will be included in the pull request including
the series.
changes from v3:
rename smp_barrier_stems to barrier_stems
as suggested by Julian Calaby.
add (?: ... ) around a variable in regexp,
in case we change the value later so that it matters.
changes from v2:
address comments by Joe Perches:
use (?: ... ) to avoid unnecessary capture groups
rename smp_barriers to smp_barrier_stems for clarity
add barriers before/after atomic
Changes from v1:
catch optional\s* before () in barriers
rewrite using qr{} instead of map
Michael S. Tsirkin (3):
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
Michael S. Tsirkin (3):
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
scripts/checkpatch.pl | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
--
MST
^ permalink raw reply
* Re: [PATCH v3 3/3] checkpatch: add virt barriers
From: Michael S. Tsirkin @ 2016-01-11 10:56 UTC (permalink / raw)
To: Julian Calaby
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Tony Lindgren,
xen-devel, Ingo Molnar, linux-xtensa, user-mode-linux-devel,
Stefano Stabellini, Andrey Konovalov, adi-buildroot-devel,
Andy Whitcroft, Thomas Gleixner, linux-metag, Mailing List, Arm
In-Reply-To: <CAGRGNgU7=vNXZbgSwLs+bBueX1+rKACCRMi74hM+jP8MaX+-WA@mail.gmail.com>
On Mon, Jan 11, 2016 at 09:40:18PM +1100, Julian Calaby wrote:
> Hi Michael,
>
> On Mon, Jan 11, 2016 at 9:35 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Jan 10, 2016 at 02:52:16PM -0800, Joe Perches wrote:
> >> On Mon, 2016-01-11 at 09:13 +1100, Julian Calaby wrote:
> >> > On Mon, Jan 11, 2016 at 6:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > > Add virt_ barriers to list of barriers to check for
> >> > > presence of a comment.
> >> []
> >> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> []
> >> > > @@ -5133,7 +5133,8 @@ sub process {
> >> > > }x;
> >> > > my $all_barriers = qr{
> >> > > $barriers|
> >> > > - smp_(?:$smp_barrier_stems)
> >> > > + smp_(?:$smp_barrier_stems)|
> >> > > + virt_(?:$smp_barrier_stems)
> >> >
> >> > Sorry I'm late to the party here, but would it make sense to write this as:
> >> >
> >> > (?:smp|virt)_(?:$smp_barrier_stems)
> >>
> >> Yes. Perhaps the name might be better as barrier_stems.
> >>
> >> Also, ideally this would be longest match first or use \b
> >> after the matches so that $all_barriers could work
> >> successfully without a following \s*\(
> >>
> >> my $all_barriers = qr{
> >> (?:smp|virt)_(?:barrier_stems)|
> >> $barriers)
> >> }x;
> >>
> >> or maybe add separate $smp_barriers and $virt_barriers
> >>
> >> <shrug> it doesn't matter much in any case
> >
> > OK just to clarify - are you OK with merging the patch as is?
> > Refactorings can come as patches on top if required.
>
> I don't really care either way, I was just asking if it was possible.
> If you don't see any value in that change, then don't make it.
>
> Thanks,
>
> --
> Julian Calaby
>
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
OK, got it, thanks.
I will rename smp_barrier_stems to barrier_stems since
this doesn't need too much testing.
I'd rather keep the regex code as is since changing it requires
testing. I might play with it some more in the future
but I'd like to merge it in the current form to help make
sure __smp barriers are not misused.
I'll post v4 now - an ack will be appreciated.
--
MST
^ permalink raw reply
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