| Message ID | 20200616045035.51641-1-pasic@linux.ibm.com |
|---|---|
| Headers | show |
| Series | two atomic_cmpxchg() related fixes | expand |
On Tue, 16 Jun 2020 06:50:33 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > The story short: compiler can generate code that does two > distinct fetches of *ind_addr for old and _old. If that happens we can > not figure out if we had the desired xchg or not. > > Halil Pasic (2): > virtio-ccw: fix virtio_set_ind_atomic > s390x/pci: fix set_ind_atomic > > hw/s390x/s390-pci-bus.c | 16 +++++++++------- > hw/s390x/virtio-ccw.c | 18 ++++++++++-------- > 2 files changed, 19 insertions(+), 15 deletions(-) > > > base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb Have we managed to reach any kind of agreement on this? (A v2?) We can still get in fixes post-softfreeze, of course.
On 01.07.20 14:01, Cornelia Huck wrote: > On Tue, 16 Jun 2020 06:50:33 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> The story short: compiler can generate code that does two >> distinct fetches of *ind_addr for old and _old. If that happens we can >> not figure out if we had the desired xchg or not. >> >> Halil Pasic (2): >> virtio-ccw: fix virtio_set_ind_atomic >> s390x/pci: fix set_ind_atomic >> >> hw/s390x/s390-pci-bus.c | 16 +++++++++------- >> hw/s390x/virtio-ccw.c | 18 ++++++++++-------- >> 2 files changed, 19 insertions(+), 15 deletions(-) >> >> >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb > > Have we managed to reach any kind of agreement on this? (A v2?) > > We can still get in fixes post-softfreeze, of course. Unless Halil has a v2 ready, I think the current patch is fine as is as a fix. I would suggest to go with that and we can then do more beautification later when necessary.
On Wed, 1 Jul 2020 14:06:11 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 01.07.20 14:01, Cornelia Huck wrote: > > On Tue, 16 Jun 2020 06:50:33 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > >> The story short: compiler can generate code that does two > >> distinct fetches of *ind_addr for old and _old. If that happens we can > >> not figure out if we had the desired xchg or not. > >> > >> Halil Pasic (2): > >> virtio-ccw: fix virtio_set_ind_atomic > >> s390x/pci: fix set_ind_atomic > >> > >> hw/s390x/s390-pci-bus.c | 16 +++++++++------- > >> hw/s390x/virtio-ccw.c | 18 ++++++++++-------- > >> 2 files changed, 19 insertions(+), 15 deletions(-) > >> > >> > >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb > > > > Have we managed to reach any kind of agreement on this? (A v2?) > > > > We can still get in fixes post-softfreeze, of course. > > Unless Halil has a v2 ready, > I think the current patch is fine as is as a fix. I would suggest > to go with that and we can then do more beautification later when > necessary. Sure, no objection to the patches as they are now. I would like to see some R-bs/A-bs, though :)
On Tue, 16 Jun 2020 06:50:33 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > The story short: compiler can generate code that does two > distinct fetches of *ind_addr for old and _old. If that happens we can > not figure out if we had the desired xchg or not. > > Halil Pasic (2): > virtio-ccw: fix virtio_set_ind_atomic > s390x/pci: fix set_ind_atomic > > hw/s390x/s390-pci-bus.c | 16 +++++++++------- > hw/s390x/virtio-ccw.c | 18 ++++++++++-------- > 2 files changed, 19 insertions(+), 15 deletions(-) > > > base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb Thanks, applied.
On Wed, 1 Jul 2020 14:06:11 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > On 01.07.20 14:01, Cornelia Huck wrote: > > On Tue, 16 Jun 2020 06:50:33 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > >> The story short: compiler can generate code that does two > >> distinct fetches of *ind_addr for old and _old. If that happens we can > >> not figure out if we had the desired xchg or not. > >> > >> Halil Pasic (2): > >> virtio-ccw: fix virtio_set_ind_atomic > >> s390x/pci: fix set_ind_atomic > >> > >> hw/s390x/s390-pci-bus.c | 16 +++++++++------- > >> hw/s390x/virtio-ccw.c | 18 ++++++++++-------- > >> 2 files changed, 19 insertions(+), 15 deletions(-) > >> > >> > >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb > > > > Have we managed to reach any kind of agreement on this? (A v2?) > > > > We can still get in fixes post-softfreeze, of course. > > Unless Halil has a v2 ready, > I think the current patch is fine as is as a fix. I would suggest > to go with that and we can then do more beautification later when > necessary. > > I think we were waiting for Paolo to chime in regarding the barrier(). The thing I could beautify is using atomic_read(), but frankly I'm not sure about it, especially considering multi-proccess. My understanding of volatile is better than my understanding of atomic_read(). On the other hand, same multi-process question can be asked about atomic_cmpxchg() and __atomic_compare_exchange_n()... Regards, Halil
On Fri, 3 Jul 2020 15:37:11 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Wed, 1 Jul 2020 14:06:11 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > > > > > On 01.07.20 14:01, Cornelia Huck wrote: > > > On Tue, 16 Jun 2020 06:50:33 +0200 > > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > >> The story short: compiler can generate code that does two > > >> distinct fetches of *ind_addr for old and _old. If that happens we can > > >> not figure out if we had the desired xchg or not. > > >> > > >> Halil Pasic (2): > > >> virtio-ccw: fix virtio_set_ind_atomic > > >> s390x/pci: fix set_ind_atomic > > >> > > >> hw/s390x/s390-pci-bus.c | 16 +++++++++------- > > >> hw/s390x/virtio-ccw.c | 18 ++++++++++-------- > > >> 2 files changed, 19 insertions(+), 15 deletions(-) > > >> > > >> > > >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb > > > > > > Have we managed to reach any kind of agreement on this? (A v2?) > > > > > > We can still get in fixes post-softfreeze, of course. > > > > Unless Halil has a v2 ready, > > I think the current patch is fine as is as a fix. I would suggest > > to go with that and we can then do more beautification later when > > necessary. > > > > > > I think we were waiting for Paolo to chime in regarding the barrier(). > The thing I could beautify is using atomic_read(), but frankly I'm not > sure about it, especially considering multi-proccess. My understanding of > volatile is better than my understanding of atomic_read(). On the other > hand, same multi-process question can be asked about atomic_cmpxchg() > and __atomic_compare_exchange_n()... We can just improve the code on top later. Having something that works now beats something not yet developed that might be nicer :) (But yes, it also might be good to look at non-s390 examples of this pattern and check if something can be done in a more central place.)