Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302269 > unrolled thread

Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-06 00:40 +0100
    Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-06 12:20 +0100
      Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Peter Zijlstra <peterz@infradead.org> - 2016-01-06 12:50 +0100
        Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-06 13:00 +0100
          Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Peter Zijlstra <peterz@infradead.org> - 2016-01-06 15:40 +0100
            Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rob Landley <rob@landley.net> - 2016-01-06 16:50 +0100
              Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Peter Zijlstra <peterz@infradead.org> - 2016-01-06 18:00 +0100
                Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rob Landley <rob@landley.net> - 2016-01-06 21:30 +0100
              Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Geert Uytterhoeven <geert@linux-m68k.org> - 2016-01-06 20:00 +0100
            Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-06 19:30 +0100
              Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-06 21:30 +0100
                Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-07 01:00 +0100
                  Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Peter Zijlstra <peterz@infradead.org> - 2016-01-07 14:40 +0100
                    Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-07 20:10 +0100
                  Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-07 17:00 +0100
                  Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-07 18:50 +0100
                    Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-07 20:20 +0100
                      Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-07 23:50 +0100
                        Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb Rich Felker <dalias@libc.org> - 2016-01-08 05:30 +0100
                          Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-08 08:30 +0100
              Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-06 23:20 +0100

Page 1 of 2  [1] 2  Next page →

#1302269 — Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb

On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
> At the moment, xchg on sh only supports 4 and 1 byte values, so using it
> from smp_store_mb means attempts to store a 2 byte value using this
> macro fail.
> 
> And happens to be exactly what virtio drivers want to do.
> 
> Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.

Can you please do this for size 1 as well (i.e. all sizes != 4)? If
you check the source, the code for size-1 xchg in sh cmpxchg-llsc.h is
completely wrong and operates on a 32-bit object at the address passed
to it. This code is presently unused anyway and I plan to submit a
patch to remove the size 1 case.

Rich
--
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/

[toc] | [next] | [standalone]

#1302684

On Tue, Jan 05, 2016 at 06:27:35PM -0500, Rich Felker wrote:
> On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
> > At the moment, xchg on sh only supports 4 and 1 byte values, so using it
> > from smp_store_mb means attempts to store a 2 byte value using this
> > macro fail.
> > 
> > And happens to be exactly what virtio drivers want to do.
> > 
> > Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
> 
> Can you please do this for size 1 as well (i.e. all sizes != 4)? If
> you check the source, the code for size-1 xchg in sh cmpxchg-llsc.h is
> completely wrong and operates on a 32-bit object at the address passed
> to it. This code is presently unused anyway and I plan to submit a
> patch to remove the size 1 case.
> 
> Rich

Ouch. And PeterZ says I should write a 2-byte xchg in asm instead,
and Fedora can't even build a full kernel for this arch at the moment :(

Peter, what do you think? How about I leave this patch as is for now?

-- 
MST
--
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/

[toc] | [prev] | [next] | [standalone]

#1302705

On Wed, Jan 06, 2016 at 01:19:44PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 05, 2016 at 06:27:35PM -0500, Rich Felker wrote:
> > On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
> > > At the moment, xchg on sh only supports 4 and 1 byte values, so using it
> > > from smp_store_mb means attempts to store a 2 byte value using this
> > > macro fail.
> > > 
> > > And happens to be exactly what virtio drivers want to do.
> > > 
> > > Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
> > 
> > Can you please do this for size 1 as well (i.e. all sizes != 4)? If
> > you check the source, the code for size-1 xchg in sh cmpxchg-llsc.h is
> > completely wrong and operates on a 32-bit object at the address passed
> > to it. This code is presently unused anyway and I plan to submit a
> > patch to remove the size 1 case.
> > 
> > Rich
> 
> Ouch. And PeterZ says I should write a 2-byte xchg in asm instead,
> and Fedora can't even build a full kernel for this arch at the moment :(

Does the kernel.org hosted cross compiler work?

> Peter, what do you think? How about I leave this patch as is for now?

No, and I object to removing the single byte implementation too. Either
remove the full arch or fix xchg() to conform. xchg() should work on all
native word sizes, for SH that would be 1,2 and 4 bytes.
--
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/

[toc] | [prev] | [next] | [standalone]

#1302706

On Wed, Jan 06, 2016 at 12:40:23PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 01:19:44PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 05, 2016 at 06:27:35PM -0500, Rich Felker wrote:
> > > On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
> > > > At the moment, xchg on sh only supports 4 and 1 byte values, so using it
> > > > from smp_store_mb means attempts to store a 2 byte value using this
> > > > macro fail.
> > > > 
> > > > And happens to be exactly what virtio drivers want to do.
> > > > 
> > > > Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
> > > 
> > > Can you please do this for size 1 as well (i.e. all sizes != 4)? If
> > > you check the source, the code for size-1 xchg in sh cmpxchg-llsc.h is
> > > completely wrong and operates on a 32-bit object at the address passed
> > > to it. This code is presently unused anyway and I plan to submit a
> > > patch to remove the size 1 case.
> > > 
> > > Rich
> > 
> > Ouch. And PeterZ says I should write a 2-byte xchg in asm instead,
> > and Fedora can't even build a full kernel for this arch at the moment :(
> 
> Does the kernel.org hosted cross compiler work?

I'll test, thanks for the hint.

> > Peter, what do you think? How about I leave this patch as is for now?
> 
> No, and I object to removing the single byte implementation too. Either
> remove the full arch or fix xchg() to conform. xchg() should work on all
> native word sizes, for SH that would be 1,2 and 4 bytes.

Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?

It does use 4 byte accesses but IIUC that is all that exists on
this architecture.


-- 
MST
--
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/

[toc] | [prev] | [next] | [standalone]

#1302822

On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > Peter, what do you think? How about I leave this patch as is for now?
> > 
> > No, and I object to removing the single byte implementation too. Either
> > remove the full arch or fix xchg() to conform. xchg() should work on all
> > native word sizes, for SH that would be 1,2 and 4 bytes.
> 
> Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?

It doesn't seem to preserve the 3 other bytes in the word.

> It does use 4 byte accesses but IIUC that is all that exists on
> this architecture.

Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
example. A store to another portion of the word should make the
store-conditional fail and we'll retry the loop.

The short versions should however preserve the other bytes in the word.

SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
versions.

In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
is always good, but ISTR some people wanting to resurrect SH:

  http://old.lwn.net/Articles/647636/

Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
take up an active interest in SH lest someone 'accidentally' nukes it?
--
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/

[toc] | [prev] | [next] | [standalone]

#1302864

On 01/06/2016 08:32 AM, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> versions.

We added a new cmpxchg() in j-core (smp on sh2 was not previously a
thing), but still need to support the old ones.

> In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> is always good, but ISTR some people wanting to resurrect SH:
> 
>   http://old.lwn.net/Articles/647636/

I note that old architectures in general become interesting again as
their patents expire and they're available for reimplementation as open
hardware. This tends to be about when the original manufacturer loses
interest, and thus people try to delete existing (working) project
support before the clones can get up to speed.

(I would have thought the presence of working QEMU support would tide us
over providing an easy basic regression testing environment, but people
keep insisting that's not real and doesn't count. But if we can keep it
99% working until the sh4 patents expire later this year, we can add mmu
and have full sh4 in hardware again with BSD VHDL.)

> Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> take up an active interest in SH lest someone 'accidentally' nukes it?

We have an active interest, we just didn't think anybody would want a
MAINTAINERS patch until we had skin in the game, and as you saw from our
previous patch the kernel code wasn't remotely acceptable upstream yet.
(Rich has been redoing it as device tree.)

We've been talking about this offline. (The superh list is cc'd on a
bunch of Renesas arm drivers because Renesas, so it has a high noise to
signal ratio.)

Sato-san agreed to co-maintain but needs somebody else to take point. (I
similarly want to assist but don't have the expertise to be the main
guy.) That leaves Rich, who is ok with doing it but was trying to finish
the device tree port of the jcore board first.

That said, if you'd ack a submission, Rich already has my Acked-by line
on a maintainers patch (AND one to remove the extra cc's from the sh
kernel list, and I acked Chen Gang's syscall addition patch back in
https://lkml.org/lkml/2015/6/20/193 but nobody noticed...)

Rich? Maintain please.

Rob
--
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/

[toc] | [prev] | [next] | [standalone]

#1302894

On Wed, Jan 06, 2016 at 09:42:35AM -0600, Rob Landley wrote:
> (I would have thought the presence of working QEMU support would tide us
> over providing an easy basic regression testing environment, but people
> keep insisting that's not real and doesn't count. But if we can keep it
> 99% working until the sh4 patents expire later this year, we can add mmu
> and have full sh4 in hardware again with BSD VHDL.)

I didn't know there was a 'working' qemu for SH.

My personal 'complaint' with SH is its lack of maintainer feedback. I do
full arch sweeps on semi-regular basis, and while I know in very board
terms how a fair number of archs work its impossible to know everything
about all 25+ we support.


--
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/

[toc] | [prev] | [next] | [standalone]

#1303040

On 01/06/2016 10:57 AM, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 09:42:35AM -0600, Rob Landley wrote:
>> (I would have thought the presence of working QEMU support would tide us
>> over providing an easy basic regression testing environment, but people
>> keep insisting that's not real and doesn't count. But if we can keep it
>> 99% working until the sh4 patents expire later this year, we can add mmu
>> and have full sh4 in hardware again with BSD VHDL.)
> 
> I didn't know there was a 'working' qemu for SH.

Yes, for several years now?

  https://lists.gnu.org/archive/html/qemu-devel/2010-03/msg00976.html

I try to build bootable images with each new kernel, although I'm a few
versions behind at the moment (this is 4.1 I think?):

  wget http://landley.net/aboriginal/bin/system-image-sh4.tar.gz
  tar xvzf system-image-sh4.tar.gz
  cd system-image-sh4
  ./run-emulator.sh

There's an sh2eb one in there too, but you need a $50 FPGA board to run
it (Numato Mimas v2, setup walkthrough is at http://nommu.org/jcore).

I keep meaning to poke at qemu and get their r2d board emulation to give
me more than 64 megs of memory so I can do native compiles. (I have a
native toolchain but building much more than "hello world" requires
setting up a swap file because the board emulation only gives me one
virtual disk. None of the other architectures need that...)

I'd _also_ like to get proper sh2 support into qemu (sh2 code runs under
sh4 but still), but the sh4 patents expire later this year and sometime
after that we want to add an MMU to the VHDL, so...

(We still want the nommu version because hard realtime is actually
easier to verify without page faults, and the big product needs
nanosecond accurate timestamps on stuff...)

> My personal 'complaint' with SH is its lack of maintainer feedback. I do
> full arch sweeps on semi-regular basis, and while I know in very board
> terms how a fair number of archs work its impossible to know everything
> about all 25+ we support.

We (the j-core guys) have wanted to take over arch/sh maintainership for
a while, we've just been trying to get the board we're working on in
position to be upstreamed first. The feedback on my craptacular first
effort to chip off a chunk that other people could at least reproduce
against a then-current kernel was "ew" and "redo it all as device tree".
So we went away again to work on that...

Meanwhile all $DAYJOB's in-house resources (at se-instruments.com) have
been tied up making SMP work for a product. (Yes, sh2 SMP. A NOMMU smp
system. There were some teething troubles, but it's working now. Alas,
not on the above $50 FPGA because that's only got an LX9 FPGA which one
SOC instance uses like 2/3 of the gates in. We're doing the SMP stuff in
LX45 boards, which are crazy expensive.)

I note I just sent a Numato board to the buildroot maintainer so I can
walk him through adding jcore support to buildroot. And I got toybox
working nommu, and Rich added sh2 support to musl-libc... There has been
activity on this arch, just not on this list due to the noise from a
bunch of different Renesas arm systems and whatever "arm/shmobile" is.
("Not superh or jcore compatible", that's all I know...)

Rob
--
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/

[toc] | [prev] | [next] | [standalone]

#1302992

On Wed, Jan 6, 2016 at 4:42 PM, Rob Landley <rob@landley.net> wrote:
> That said, if you'd ack a submission, Rich already has my Acked-by line
> on a maintainers patch (AND one to remove the extra cc's from the sh
> kernel list, and I acked Chen Gang's syscall addition patch back in
> https://lkml.org/lkml/2015/6/20/193 but nobody noticed...)

If you want patches for orphan architectures to be picked up, you should
CC Andrew Morton...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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/

[toc] | [prev] | [next] | [standalone]

#1302967

On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > Peter, what do you think? How about I leave this patch as is for now?
> > > 
> > > No, and I object to removing the single byte implementation too. Either
> > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > 
> > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> 
> It doesn't seem to preserve the 3 other bytes in the word.
> 
> > It does use 4 byte accesses but IIUC that is all that exists on
> > this architecture.
> 
> Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> example. A store to another portion of the word should make the
> store-conditional fail and we'll retry the loop.
> 
> The short versions should however preserve the other bytes in the word.

Indeed. Also, accesses must be aligned, so the asm needs to round down
to an aligned address and perform a correct read-modify-write on it,
placing the new byte in the correct offset in the word.

Alternatively (my preference) this logic can be impemented in C as a
wrapper around the 32-bit cmpxchg. I think this is less error-prone
and it can be shared between the multiple sh cmpxchg back-ends,
including the new cas.l one we need for J2.

> SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> versions.
> 
> In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> is always good, but ISTR some people wanting to resurrect SH:
> 
>   http://old.lwn.net/Articles/647636/
> 
> Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> take up an active interest in SH lest someone 'accidentally' nukes it?

We're in the process of preparing such a proposal right now. That
current intent is that Sato-san and I will co-maintain arch/sh. We'll
include more details about motivation, proposed development direction,
existing work to be merged, etc. in that proposal.

Rich
--
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/

[toc] | [prev] | [next] | [standalone]

#1303039

On Wed, Jan 06, 2016 at 01:23:50PM -0500, Rich Felker wrote:
> On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > > Peter, what do you think? How about I leave this patch as is for now?
> > > > 
> > > > No, and I object to removing the single byte implementation too. Either
> > > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > > 
> > > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> > 
> > It doesn't seem to preserve the 3 other bytes in the word.
> > 
> > > It does use 4 byte accesses but IIUC that is all that exists on
> > > this architecture.
> > 
> > Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> > example. A store to another portion of the word should make the
> > store-conditional fail and we'll retry the loop.
> > 
> > The short versions should however preserve the other bytes in the word.
> 
> Indeed. Also, accesses must be aligned, so the asm needs to round down
> to an aligned address and perform a correct read-modify-write on it,
> placing the new byte in the correct offset in the word.
> 
> Alternatively (my preference) this logic can be impemented in C as a
> wrapper around the 32-bit cmpxchg. I think this is less error-prone
> and it can be shared between the multiple sh cmpxchg back-ends,
> including the new cas.l one we need for J2.
> 
> > SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> > versions.
> > 
> > In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> > is always good, but ISTR some people wanting to resurrect SH:
> > 
> >   http://old.lwn.net/Articles/647636/
> > 
> > Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> > take up an active interest in SH lest someone 'accidentally' nukes it?
> 
> We're in the process of preparing such a proposal right now. That
> current intent is that Sato-san and I will co-maintain arch/sh. We'll
> include more details about motivation, proposed development direction,
> existing work to be merged, etc. in that proposal.
> 
> Rich

Well I'd like to be able to make progress with generic
arch cleanups meanwhile.

Could you quickly write a version of 1 and 2 byte xchg that
works so I can include it?

Thanks!

-- 
MST
--
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/

[toc] | [prev] | [next] | [standalone]

#1303160

On Wed, Jan 06, 2016 at 10:23:12PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 06, 2016 at 01:23:50PM -0500, Rich Felker wrote:
> > On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > > > Peter, what do you think? How about I leave this patch as is for now?
> > > > > 
> > > > > No, and I object to removing the single byte implementation too. Either
> > > > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > > > 
> > > > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> > > 
> > > It doesn't seem to preserve the 3 other bytes in the word.
> > > 
> > > > It does use 4 byte accesses but IIUC that is all that exists on
> > > > this architecture.
> > > 
> > > Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> > > example. A store to another portion of the word should make the
> > > store-conditional fail and we'll retry the loop.
> > > 
> > > The short versions should however preserve the other bytes in the word.
> > 
> > Indeed. Also, accesses must be aligned, so the asm needs to round down
> > to an aligned address and perform a correct read-modify-write on it,
> > placing the new byte in the correct offset in the word.
> > 
> > Alternatively (my preference) this logic can be impemented in C as a
> > wrapper around the 32-bit cmpxchg. I think this is less error-prone
> > and it can be shared between the multiple sh cmpxchg back-ends,
> > including the new cas.l one we need for J2.
> > 
> > > SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> > > versions.
> > > 
> > > In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> > > is always good, but ISTR some people wanting to resurrect SH:
> > > 
> > >   http://old.lwn.net/Articles/647636/
> > > 
> > > Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> > > take up an active interest in SH lest someone 'accidentally' nukes it?
> > 
> > We're in the process of preparing such a proposal right now. That
> > current intent is that Sato-san and I will co-maintain arch/sh. We'll
> > include more details about motivation, proposed development direction,
> > existing work to be merged, etc. in that proposal.
> 
> Well I'd like to be able to make progress with generic
> arch cleanups meanwhile.
> 
> Could you quickly write a version of 1 and 2 byte xchg that
> works so I can include it?

Here are quick, untested generic ones:

static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
{
	u32 old;
	unsigned long offset = (unsigned long)m & 3;
	volatile u32 *w = (volatile u32 *)(m - offset);
	union { u32 w; u8 b[4]; } u;
	do {
		old = u.w = *w;
		result = w.b[offset];
		w.b[offset] = val;
	} while (cmpxchg(w, old, u.w) != old);
	return result;
}

static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
{
	u32 old;
	unsigned long result;
	unsigned long offset = ((unsigned long)m & 3) >> 1;
	volatile u32 *w = (volatile u32 *)(m - offset);
	union { u32 w; u16 h[2]; } u;
	do {
		old = u.w = *w;
		result = w.h[offset];
		w.h[offset] = val;
	} while (cmpxchg(w, old, u.w) != old);
	return result;
}

It would be nice to have these in asm-generic for archs which don't
define their own versions rather than having cruft like this repeated
per-arch. Strictly speaking, the volatile u32 used to access the
32-bit word containing the u8 or u16 should be
__attribute__((__may_alias__)) too. Is there an existing kernel type
for a "may_alias u32" or should it perhaps be added?

Rich
--
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/

[toc] | [prev] | [next] | [standalone]

#1303593

On Wed, Jan 06, 2016 at 06:53:01PM -0500, Rich Felker wrote:
> It would be nice to have these in asm-generic for archs which don't
> define their own versions rather than having cruft like this repeated
> per-arch. 

Maybe, but I'm not sure how many archs would indeed suffer this problem,
so far I'm only aware of Alpha and SH that do not have short atomic ops.

> Strictly speaking, the volatile u32 used to access the
> 32-bit word containing the u8 or u16 should be
> __attribute__((__may_alias__)) too. Is there an existing kernel type
> for a "may_alias u32" or should it perhaps be added?

The kernel does -fno-strict-aliasing because the C aliasing rules are
crap (TM) :-), so I suspect we do not need the alias attribute here.
--
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/

[toc] | [prev] | [next] | [standalone]

#1303812

On Thu, Jan 07, 2016 at 02:37:49PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 06:53:01PM -0500, Rich Felker wrote:
> > It would be nice to have these in asm-generic for archs which don't
> > define their own versions rather than having cruft like this repeated
> > per-arch. 
> 
> Maybe, but I'm not sure how many archs would indeed suffer this problem,
> so far I'm only aware of Alpha and SH that do not have short atomic ops.

Apparently original armv6 (non-k) lacked u8 and u16 variants of
ldrex/strex. I'm pretty sure or1k also lacks them, and mips,
microblaze, and some powerpc versions might too. Not sure about
risc-v.

> > Strictly speaking, the volatile u32 used to access the
> > 32-bit word containing the u8 or u16 should be
> > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > for a "may_alias u32" or should it perhaps be added?
> 
> The kernel does -fno-strict-aliasing because the C aliasing rules are
> crap (TM) :-), so I suspect we do not need the alias attribute here.

I suspect working on the kernel I'm going to have to get used to
getting "corrected" for writing proper C... ;-)

Rich

[toc] | [prev] | [next] | [standalone]

#1303701

On Wed, Jan 06, 2016 at 06:53:01PM -0500, Rich Felker wrote:
> On Wed, Jan 06, 2016 at 10:23:12PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 06, 2016 at 01:23:50PM -0500, Rich Felker wrote:
> > > On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > > > > Peter, what do you think? How about I leave this patch as is for now?
> > > > > > 
> > > > > > No, and I object to removing the single byte implementation too. Either
> > > > > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > > > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > > > > 
> > > > > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> > > > 
> > > > It doesn't seem to preserve the 3 other bytes in the word.
> > > > 
> > > > > It does use 4 byte accesses but IIUC that is all that exists on
> > > > > this architecture.
> > > > 
> > > > Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> > > > example. A store to another portion of the word should make the
> > > > store-conditional fail and we'll retry the loop.
> > > > 
> > > > The short versions should however preserve the other bytes in the word.
> > > 
> > > Indeed. Also, accesses must be aligned, so the asm needs to round down
> > > to an aligned address and perform a correct read-modify-write on it,
> > > placing the new byte in the correct offset in the word.
> > > 
> > > Alternatively (my preference) this logic can be impemented in C as a
> > > wrapper around the 32-bit cmpxchg. I think this is less error-prone
> > > and it can be shared between the multiple sh cmpxchg back-ends,
> > > including the new cas.l one we need for J2.
> > > 
> > > > SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> > > > versions.
> > > > 
> > > > In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> > > > is always good, but ISTR some people wanting to resurrect SH:
> > > > 
> > > >   http://old.lwn.net/Articles/647636/
> > > > 
> > > > Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> > > > take up an active interest in SH lest someone 'accidentally' nukes it?
> > > 
> > > We're in the process of preparing such a proposal right now. That
> > > current intent is that Sato-san and I will co-maintain arch/sh. We'll
> > > include more details about motivation, proposed development direction,
> > > existing work to be merged, etc. in that proposal.
> > 
> > Well I'd like to be able to make progress with generic
> > arch cleanups meanwhile.
> > 
> > Could you quickly write a version of 1 and 2 byte xchg that
> > works so I can include it?
> 
> Here are quick, untested generic ones:
> 
> static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
> {
> 	u32 old;
> 	unsigned long offset = (unsigned long)m & 3;
> 	volatile u32 *w = (volatile u32 *)(m - offset);
> 	union { u32 w; u8 b[4]; } u;
> 	do {
> 		old = u.w = *w;
> 		result = w.b[offset];
> 		w.b[offset] = val;
> 	} while (cmpxchg(w, old, u.w) != old);
> 	return result;
> }
> 
> static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
> {
> 	u32 old;
> 	unsigned long result;
> 	unsigned long offset = ((unsigned long)m & 3) >> 1;
> 	volatile u32 *w = (volatile u32 *)(m - offset);
> 	union { u32 w; u16 h[2]; } u;
> 	do {
> 		old = u.w = *w;
> 		result = w.h[offset];
> 		w.h[offset] = val;
> 	} while (cmpxchg(w, old, u.w) != old);
> 	return result;
> }
> 
> It would be nice to have these in asm-generic for archs which don't
> define their own versions rather than having cruft like this repeated
> per-arch. Strictly speaking, the volatile u32 used to access the
> 32-bit word containing the u8 or u16 should be
> __attribute__((__may_alias__)) too.
> Is there an existing kernel type
> for a "may_alias u32" or should it perhaps be added?
> 
> Rich

I'm inclined to write this using shifts, this way there's
no duplication between 1 and 2 byte variants.
And READ_ONCE is better than volatile IMHO.


diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
index 85c97b18..a858879 100644
--- a/arch/sh/include/asm/cmpxchg.h
+++ b/arch/sh/include/asm/cmpxchg.h
@@ -8,6 +8,7 @@
 
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <asm/byteorder.h>
 
 #if defined(CONFIG_GUSA_RB)
 #include <asm/cmpxchg-grb.h>
@@ -19,6 +20,26 @@
 
 extern void __xchg_called_with_bad_pointer(void);
 
+static inline u32 __xchg_cmpxchg(void *ptr, u32 x, int size)
+{
+	int off = (unsigned long)ptr % sizeof(u32);
+	u32 *p = ptr - off;
+	int bitoff = __BYTE_ORDER == __BIG_ENDIAN ?
+		((sizeof(u32) - 1 - off) * BITS_PER_BYTE) :
+		(off * BITS_PER_BYTE);
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;
+	u32 oldv, newv;
+	u32 ret;
+
+	do {
+		oldv = READ_ONCE(*p);
+		ret = (oldv & bitmask) >> bitoff;
+		newv = (oldv & ~bitmask) | (x << bitoff);
+	} while(cmpxchg(p, oldv, newv) != oldv);
+
+	return ret;
+}
+
 #define __xchg(ptr, x, size)				\
 ({							\
 	unsigned long __xchg__res;			\
@@ -27,8 +48,10 @@ extern void __xchg_called_with_bad_pointer(void);
 	case 4:						\
 		__xchg__res = xchg_u32(__xchg_ptr, x);	\
 		break;					\
+	case 2:						\
 	case 1:						\
-		__xchg__res = xchg_u8(__xchg_ptr, x);	\
+		__xchg__res = __xchg_cmpxchg(__xchg_ptr,\
+					     x, size);	\
 		break;					\
 	default:					\
 		__xchg_called_with_bad_pointer();	\


Testing the above now.
-- 
MST
--
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/

[toc] | [prev] | [next] | [standalone]

#1303766

On Wed, Jan 06, 2016 at 06:53:01PM -0500, Rich Felker wrote:
> On Wed, Jan 06, 2016 at 10:23:12PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 06, 2016 at 01:23:50PM -0500, Rich Felker wrote:
> > > On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > > > > Peter, what do you think? How about I leave this patch as is for now?
> > > > > > 
> > > > > > No, and I object to removing the single byte implementation too. Either
> > > > > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > > > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > > > > 
> > > > > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> > > > 
> > > > It doesn't seem to preserve the 3 other bytes in the word.
> > > > 
> > > > > It does use 4 byte accesses but IIUC that is all that exists on
> > > > > this architecture.
> > > > 
> > > > Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> > > > example. A store to another portion of the word should make the
> > > > store-conditional fail and we'll retry the loop.
> > > > 
> > > > The short versions should however preserve the other bytes in the word.
> > > 
> > > Indeed. Also, accesses must be aligned, so the asm needs to round down
> > > to an aligned address and perform a correct read-modify-write on it,
> > > placing the new byte in the correct offset in the word.
> > > 
> > > Alternatively (my preference) this logic can be impemented in C as a
> > > wrapper around the 32-bit cmpxchg. I think this is less error-prone
> > > and it can be shared between the multiple sh cmpxchg back-ends,
> > > including the new cas.l one we need for J2.
> > > 
> > > > SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> > > > versions.
> > > > 
> > > > In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> > > > is always good, but ISTR some people wanting to resurrect SH:
> > > > 
> > > >   http://old.lwn.net/Articles/647636/
> > > > 
> > > > Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> > > > take up an active interest in SH lest someone 'accidentally' nukes it?
> > > 
> > > We're in the process of preparing such a proposal right now. That
> > > current intent is that Sato-san and I will co-maintain arch/sh. We'll
> > > include more details about motivation, proposed development direction,
> > > existing work to be merged, etc. in that proposal.
> > 
> > Well I'd like to be able to make progress with generic
> > arch cleanups meanwhile.
> > 
> > Could you quickly write a version of 1 and 2 byte xchg that
> > works so I can include it?
> 
> Here are quick, untested generic ones:
> 
> static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
> {
> 	u32 old;
> 	unsigned long offset = (unsigned long)m & 3;
> 	volatile u32 *w = (volatile u32 *)(m - offset);
> 	union { u32 w; u8 b[4]; } u;
> 	do {
> 		old = u.w = *w;
> 		result = w.b[offset];
> 		w.b[offset] = val;
> 	} while (cmpxchg(w, old, u.w) != old);
> 	return result;
> }
> 
> static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
> {
> 	u32 old;
> 	unsigned long result;
> 	unsigned long offset = ((unsigned long)m & 3) >> 1;
> 	volatile u32 *w = (volatile u32 *)(m - offset);
> 	union { u32 w; u16 h[2]; } u;
> 	do {
> 		old = u.w = *w;
> 		result = w.h[offset];
> 		w.h[offset] = val;
> 	} while (cmpxchg(w, old, u.w) != old);
> 	return result;
> }
> 
> It would be nice to have these in asm-generic for archs which don't
> define their own versions rather than having cruft like this repeated
> per-arch. Strictly speaking, the volatile u32 used to access the
> 32-bit word containing the u8 or u16 should be
> __attribute__((__may_alias__)) too. Is there an existing kernel type
> for a "may_alias u32" or should it perhaps be added?
> 
> Rich

BTW this seems suboptimal for grb and irq variants which apparently
can do things correctly.

[toc] | [prev] | [next] | [standalone]

#1303823

On Thu, Jan 07, 2016 at 07:48:08PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 06, 2016 at 06:53:01PM -0500, Rich Felker wrote:
> > On Wed, Jan 06, 2016 at 10:23:12PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jan 06, 2016 at 01:23:50PM -0500, Rich Felker wrote:
> > > > On Wed, Jan 06, 2016 at 03:32:18PM +0100, Peter Zijlstra wrote:
> > > > > On Wed, Jan 06, 2016 at 01:52:17PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > Peter, what do you think? How about I leave this patch as is for now?
> > > > > > > 
> > > > > > > No, and I object to removing the single byte implementation too. Either
> > > > > > > remove the full arch or fix xchg() to conform. xchg() should work on all
> > > > > > > native word sizes, for SH that would be 1,2 and 4 bytes.
> > > > > > 
> > > > > > Rick, maybe you could explain how is current 1 byte xchg on llsc wrong?
> > > > > 
> > > > > It doesn't seem to preserve the 3 other bytes in the word.
> > > > > 
> > > > > > It does use 4 byte accesses but IIUC that is all that exists on
> > > > > > this architecture.
> > > > > 
> > > > > Right, that's not a problem, look at arch/alpha/include/asm/xchg.h for
> > > > > example. A store to another portion of the word should make the
> > > > > store-conditional fail and we'll retry the loop.
> > > > > 
> > > > > The short versions should however preserve the other bytes in the word.
> > > > 
> > > > Indeed. Also, accesses must be aligned, so the asm needs to round down
> > > > to an aligned address and perform a correct read-modify-write on it,
> > > > placing the new byte in the correct offset in the word.
> > > > 
> > > > Alternatively (my preference) this logic can be impemented in C as a
> > > > wrapper around the 32-bit cmpxchg. I think this is less error-prone
> > > > and it can be shared between the multiple sh cmpxchg back-ends,
> > > > including the new cas.l one we need for J2.
> > > > 
> > > > > SH's cmpxchg() is equally incomplete and does not provide 1 and 2 byte
> > > > > versions.
> > > > > 
> > > > > In any case, I'm all for rm -rf arch/sh/, one less arch to worry about
> > > > > is always good, but ISTR some people wanting to resurrect SH:
> > > > > 
> > > > >   http://old.lwn.net/Articles/647636/
> > > > > 
> > > > > Rob, Jeff, Sato-san, might I suggest you send a MAINTAINERS patch and
> > > > > take up an active interest in SH lest someone 'accidentally' nukes it?
> > > > 
> > > > We're in the process of preparing such a proposal right now. That
> > > > current intent is that Sato-san and I will co-maintain arch/sh. We'll
> > > > include more details about motivation, proposed development direction,
> > > > existing work to be merged, etc. in that proposal.
> > > 
> > > Well I'd like to be able to make progress with generic
> > > arch cleanups meanwhile.
> > > 
> > > Could you quickly write a version of 1 and 2 byte xchg that
> > > works so I can include it?
> > 
> > Here are quick, untested generic ones:
> > 
> > static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
> > {
> > 	u32 old;
> > 	unsigned long offset = (unsigned long)m & 3;
> > 	volatile u32 *w = (volatile u32 *)(m - offset);
> > 	union { u32 w; u8 b[4]; } u;
> > 	do {
> > 		old = u.w = *w;
> > 		result = w.b[offset];
> > 		w.b[offset] = val;
> > 	} while (cmpxchg(w, old, u.w) != old);
> > 	return result;
> > }
> > 
> > static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
> > {
> > 	u32 old;
> > 	unsigned long result;
> > 	unsigned long offset = ((unsigned long)m & 3) >> 1;
> > 	volatile u32 *w = (volatile u32 *)(m - offset);
> > 	union { u32 w; u16 h[2]; } u;
> > 	do {
> > 		old = u.w = *w;
> > 		result = w.h[offset];
> > 		w.h[offset] = val;
> > 	} while (cmpxchg(w, old, u.w) != old);
> > 	return result;
> > }
> > 
> > It would be nice to have these in asm-generic for archs which don't
> > define their own versions rather than having cruft like this repeated
> > per-arch. Strictly speaking, the volatile u32 used to access the
> > 32-bit word containing the u8 or u16 should be
> > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > for a "may_alias u32" or should it perhaps be added?
> > 
> > Rich
> 
> BTW this seems suboptimal for grb and irq variants which apparently
> can do things correctly.

In principle I agree, but u8/u16 xchg is mostly unused (completely
unused in my builds) and unlikely to matter to performance. Also, the
irq variant is only for the original sh2 which is not even produced
anymore afaik. Our reimplementation of the sh2 ISA, the J2, has a
cas.l instruction that will be used instead because it supports SMP
where interrupt masking is insufficient to achieve atomicity.

Rich

[toc] | [prev] | [next] | [standalone]

#1303968

> > > It would be nice to have these in asm-generic for archs which don't
> > > define their own versions rather than having cruft like this repeated
> > > per-arch. Strictly speaking, the volatile u32 used to access the
> > > 32-bit word containing the u8 or u16 should be
> > > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > > for a "may_alias u32" or should it perhaps be added?
> > > 
> > > Rich
> > 
> > BTW this seems suboptimal for grb and irq variants which apparently
> > can do things correctly.
> 
> In principle I agree, but u8/u16 xchg is mostly unused (completely
> unused in my builds) and unlikely to matter to performance. Also, the
> irq variant is only for the original sh2 which is not even produced
> anymore afaik. Our reimplementation of the sh2 ISA, the J2, has a
> cas.l instruction that will be used instead because it supports SMP
> where interrupt masking is insufficient to achieve atomicity.
> 
> Rich

Since it looks like there will soon be active maintainers
for this arch, I think it's best if I make the minimal possible
changes and then you guys can rewrite it any way you like,
drop irq variant or whatever.

The minimal change is probably the below code but
the grb variant is just copy paste from xchg_u8
with a minor tweak -
can you pls confirm it looks right?

I tested the llsc code on ppc and x86 and since it's
portable I know the logic is correct there.

Will post v3 with this included but would appreciate
your input first.

---->
sh: support 1 and 2 byte xchg

This completes the xchg implementation for sh architecture.  Note: The
llsc variant is tricky since this only supports 4 byte atomics, the
existing implementation of 1 byte xchg is wrong: we need to do a 4 byte
cmpxchg and retry if any bytes changed meanwhile.

Write this in C for clarity.

Suggested-by: Rich Felker <dalias@libc.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---->

diff --git a/arch/sh/include/asm/cmpxchg-grb.h b/arch/sh/include/asm/cmpxchg-grb.h
index f848dec..2ed557b 100644
--- a/arch/sh/include/asm/cmpxchg-grb.h
+++ b/arch/sh/include/asm/cmpxchg-grb.h
@@ -23,6 +23,28 @@ static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 	return retval;
 }
 
+static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
+{
+	unsigned long retval;
+
+	__asm__ __volatile__ (
+		"   .align  2             \n\t"
+		"   mova    1f,   r0      \n\t" /* r0 = end point */
+		"   mov    r15,   r1      \n\t" /* r1 = saved sp */
+		"   mov    #-6,   r15     \n\t" /* LOGIN */
+		"   mov.w  @%1,   %0      \n\t" /* load  old value */
+		"   extu.w  %0,   %0      \n\t" /* extend as unsigned */
+		"   mov.w   %2,   @%1     \n\t" /* store new value */
+		"1: mov     r1,   r15     \n\t" /* LOGOUT */
+		: "=&r" (retval),
+		  "+r"  (m),
+		  "+r"  (val)		/* inhibit r15 overloading */
+		:
+		: "memory" , "r0", "r1");
+
+	return retval;
+}
+
 static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
 {
 	unsigned long retval;
diff --git a/arch/sh/include/asm/cmpxchg-irq.h b/arch/sh/include/asm/cmpxchg-irq.h
index bd11f63..f888772 100644
--- a/arch/sh/include/asm/cmpxchg-irq.h
+++ b/arch/sh/include/asm/cmpxchg-irq.h
@@ -14,6 +14,17 @@ static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 	return retval;
 }
 
+static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
+{
+	unsigned long flags, retval;
+
+	local_irq_save(flags);
+	retval = *m;
+	*m = val;
+	local_irq_restore(flags);
+	return retval;
+}
+
 static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
 {
 	unsigned long flags, retval;
diff --git a/arch/sh/include/asm/cmpxchg-llsc.h b/arch/sh/include/asm/cmpxchg-llsc.h
index 4713666..5dfdb06 100644
--- a/arch/sh/include/asm/cmpxchg-llsc.h
+++ b/arch/sh/include/asm/cmpxchg-llsc.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_SH_CMPXCHG_LLSC_H
 #define __ASM_SH_CMPXCHG_LLSC_H
 
+#include <asm/byteorder.h>
+
 static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 {
 	unsigned long retval;
@@ -22,29 +24,8 @@ static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 	return retval;
 }
 
-static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
-{
-	unsigned long retval;
-	unsigned long tmp;
-
-	__asm__ __volatile__ (
-		"1:					\n\t"
-		"movli.l	@%2, %0	! xchg_u8	\n\t"
-		"mov		%0, %1			\n\t"
-		"mov		%3, %0			\n\t"
-		"movco.l	%0, @%2			\n\t"
-		"bf		1b			\n\t"
-		"synco					\n\t"
-		: "=&z"(tmp), "=&r" (retval)
-		: "r" (m), "r" (val & 0xff)
-		: "t", "memory"
-	);
-
-	return retval;
-}
-
 static inline unsigned long
-__cmpxchg_u32(volatile int *m, unsigned long old, unsigned long new)
+__cmpxchg_u32(volatile u32 *m, unsigned long old, unsigned long new)
 {
 	unsigned long retval;
 	unsigned long tmp;
@@ -68,4 +49,34 @@ __cmpxchg_u32(volatile int *m, unsigned long old, unsigned long new)
 	return retval;
 }
 
+static inline u32 __xchg_cmpxchg(volatile void *ptr, u32 x, int size)
+{
+	int off = (unsigned long)ptr % sizeof(u32);
+	volatile u32 *p = ptr - off;
+	int bitoff = __BYTE_ORDER == __BIG_ENDIAN ?
+		((sizeof(u32) - 1 - off) * BITS_PER_BYTE) :
+		(off * BITS_PER_BYTE);
+	u32 bitmask = ((0x1 << size * BITS_PER_BYTE) - 1) << bitoff;
+	u32 oldv, newv;
+	u32 ret;
+
+	do {
+		oldv = READ_ONCE(*p);
+		ret = (oldv & bitmask) >> bitoff;
+		newv = (oldv & ~bitmask) | (x << bitoff);
+	} while (__cmpxchg_u32(p, oldv, newv) != oldv);
+
+	return ret;
+}
+
+static inline unsigned long xchg_u16(volatile u16 *m, unsigned long val)
+{
+	return __xchg_cmpxchg(m, val, sizeof *m);
+}
+
+static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
+{
+	return __xchg_cmpxchg(m, val, sizeof *m);
+}
+
 #endif /* __ASM_SH_CMPXCHG_LLSC_H */
diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
index 85c97b18..5225916 100644
--- a/arch/sh/include/asm/cmpxchg.h
+++ b/arch/sh/include/asm/cmpxchg.h
@@ -27,6 +27,9 @@ extern void __xchg_called_with_bad_pointer(void);
 	case 4:						\
 		__xchg__res = xchg_u32(__xchg_ptr, x);	\
 		break;					\
+	case 2:						\
+		__xchg__res = xchg_u16(__xchg_ptr, x);	\
+		break;					\
 	case 1:						\
 		__xchg__res = xchg_u8(__xchg_ptr, x);	\
 		break;					\

[toc] | [prev] | [next] | [standalone]

#1304165

On Fri, Jan 08, 2016 at 12:41:35AM +0200, Michael S. Tsirkin wrote:
> > > > It would be nice to have these in asm-generic for archs which don't
> > > > define their own versions rather than having cruft like this repeated
> > > > per-arch. Strictly speaking, the volatile u32 used to access the
> > > > 32-bit word containing the u8 or u16 should be
> > > > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > > > for a "may_alias u32" or should it perhaps be added?
> > > > 
> > > > Rich
> > > 
> > > BTW this seems suboptimal for grb and irq variants which apparently
> > > can do things correctly.
> > 
> > In principle I agree, but u8/u16 xchg is mostly unused (completely
> > unused in my builds) and unlikely to matter to performance. Also, the
> > irq variant is only for the original sh2 which is not even produced
> > anymore afaik. Our reimplementation of the sh2 ISA, the J2, has a
> > cas.l instruction that will be used instead because it supports SMP
> > where interrupt masking is insufficient to achieve atomicity.
> > 
> > Rich
> 
> Since it looks like there will soon be active maintainers
> for this arch, I think it's best if I make the minimal possible
> changes and then you guys can rewrite it any way you like,
> drop irq variant or whatever.
> 
> The minimal change is probably the below code but
> the grb variant is just copy paste from xchg_u8
> with a minor tweak -
> can you pls confirm it looks right?

I haven't had a chance to test it, but I don't see anything obviously
wrong with it.

> I tested the llsc code on ppc and x86 and since it's
> portable I know the logic is correct there.

Sounds good. Since it will also be needed for the cas.l variant I'd
rather have this in the main asm/cmpxchg.h where it can be shared if
you see an easy way to do that now, but if not I can take care of it
later when merging cmpxchg-cas.h. Perhaps just putting __xchg_cmpxchg
in the main asm/cmpxchg.h would suffice so that only the thin wrappers
need to be duplicated. Ideally it could even be moved outside of the
arch asm headers, but then there might be annoying header ordering
issues to deal with.

> Will post v3 with this included but would appreciate
> your input first.

Go ahead. Thanks!

Rich

[toc] | [prev] | [next] | [standalone]

#1304233

On Thu, Jan 07, 2016 at 11:25:05PM -0500, Rich Felker wrote:
> On Fri, Jan 08, 2016 at 12:41:35AM +0200, Michael S. Tsirkin wrote:
> > > > > It would be nice to have these in asm-generic for archs which don't
> > > > > define their own versions rather than having cruft like this repeated
> > > > > per-arch. Strictly speaking, the volatile u32 used to access the
> > > > > 32-bit word containing the u8 or u16 should be
> > > > > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > > > > for a "may_alias u32" or should it perhaps be added?
> > > > > 
> > > > > Rich
> > > > 
> > > > BTW this seems suboptimal for grb and irq variants which apparently
> > > > can do things correctly.
> > > 
> > > In principle I agree, but u8/u16 xchg is mostly unused (completely
> > > unused in my builds) and unlikely to matter to performance. Also, the
> > > irq variant is only for the original sh2 which is not even produced
> > > anymore afaik. Our reimplementation of the sh2 ISA, the J2, has a
> > > cas.l instruction that will be used instead because it supports SMP
> > > where interrupt masking is insufficient to achieve atomicity.
> > > 
> > > Rich
> > 
> > Since it looks like there will soon be active maintainers
> > for this arch, I think it's best if I make the minimal possible
> > changes and then you guys can rewrite it any way you like,
> > drop irq variant or whatever.
> > 
> > The minimal change is probably the below code but
> > the grb variant is just copy paste from xchg_u8
> > with a minor tweak -
> > can you pls confirm it looks right?
> 
> I haven't had a chance to test it, but I don't see anything obviously
> wrong with it.
> 
> > I tested the llsc code on ppc and x86 and since it's
> > portable I know the logic is correct there.
> 
> Sounds good. Since it will also be needed for the cas.l variant I'd
> rather have this in the main asm/cmpxchg.h where it can be shared if
> you see an easy way to do that now, but if not I can take care of it
> later when merging cmpxchg-cas.h. Perhaps just putting __xchg_cmpxchg
> in the main asm/cmpxchg.h would suffice so that only the thin wrappers
> need to be duplicated.
> Ideally it could even be moved outside of the
> arch asm headers, but then there might be annoying header ordering
> issues to deal with.

Well it isn't possible to put it in cmpxchg.h because you get into
annoying ordering issues: __cmpxchg_u32 is needed
so it has to come after the headers, but the wrappers must
come before the headers.

I put it in a header by itself. This way it's easy to reuse,
and even the thin wrappers won't have to be duplicated.

> 
> > Will post v3 with this included but would appreciate
> > your input first.
> 
> Go ahead. Thanks!
> 
> Rich

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web