| Message ID | 20190704151522.32639-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros | expand |
On 04.07.2019 17:15, Anthony PERARD wrote: > Those macros where introduced when a prefix "xen_" was added to > mb,rmb,wmb. There are gated on __XEN_INTERFACE_VERSION__, but there > are not part of the Xen interface. Users of ring.h needs to provide > xen_[rw]?mb() anywai because [rw]?mb() isn't likely to exist. It's not clear to me what you want to achieve: > @@ -42,14 +49,6 @@ > * and grant_table.h from the Xen public headers. > */ > > -#include "../xen-compat.h" > - > -#if __XEN_INTERFACE_VERSION__ < 0x00030208 > -#define xen_mb() mb() > -#define xen_rmb() rmb() > -#define xen_wmb() wmb() > -#endif They're already not polluting the name space for any modern consumer. And you're risking to break old verbatim users of the header. Such compatibility stuff can, in my opinion, simply never go away. Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 04 July 2019 16:49 > To: Anthony Perard <anthony.perard@citrix.com> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Juergen Gross <JGross@suse.com> > Subject: Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros > > On 04.07.2019 17:15, Anthony PERARD wrote: > > Those macros where introduced when a prefix "xen_" was added to > > mb,rmb,wmb. There are gated on __XEN_INTERFACE_VERSION__, but there > > are not part of the Xen interface. Users of ring.h needs to provide > > xen_[rw]?mb() anywai because [rw]?mb() isn't likely to exist. > > It's not clear to me what you want to achieve: > The issue is that any project importing this header (in this case QEMU, but I have the same issue in the Windows PV drivers) needs to import xen-compat.h (or dream up a header of the same name), even though this header is only concerned with the underpinnings of PV protocols and has nothing, as such, to do with Xen. To keep old verbatim users (are there really any at all?) happy, how about simple... #ifndef xen_mb() #define xen_mb() mb() #endif constructs? Paul
On 04.07.2019 18:11, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <JBeulich@suse.com> >> Sent: 04 July 2019 16:49 >> To: Anthony Perard <anthony.perard@citrix.com> >> Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Konrad Rzeszutek Wilk >> <konrad.wilk@oracle.com>; Juergen Gross <JGross@suse.com> >> Subject: Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros >> >> On 04.07.2019 17:15, Anthony PERARD wrote: >>> Those macros where introduced when a prefix "xen_" was added to >>> mb,rmb,wmb. There are gated on __XEN_INTERFACE_VERSION__, but there >>> are not part of the Xen interface. Users of ring.h needs to provide >>> xen_[rw]?mb() anywai because [rw]?mb() isn't likely to exist. >> >> It's not clear to me what you want to achieve: >> > > The issue is that any project importing this header (in this case QEMU, > but I have the same issue in the Windows PV drivers) needs to import > xen-compat.h (or dream up a header of the same name), even though this > header is only concerned with the underpinnings of PV protocols and has > nothing, as such, to do with Xen. While I agree this shouldn't have been part of the public headers, that ship has sailed long, long ago. If a component doesn't use the headers verbatim, I don't see why they couldn't remove that section in their copy. If otoh they use the headers verbatim, then I'd expect them to also use xen-compat.h > To keep old verbatim users (are there really any at all?) happy, how about simple... > > #ifndef xen_mb() > #define xen_mb() mb() > #endif > > constructs? This would still cause conflicts if a component ends up defining xen_mb() only after the inclusion of this header. As to there actually being any - the old Linux 2.6.18 tree did pull in copies of the headers without further editing. Beyond that while I'm unaware of any, we simply can't know. Until now there simply was an implied promise that we would try our best to avoid breaking existing users. As a community we could certainly decide that we don't care doing so anymore, at which point more compat cruft could be deleted. I wouldn't support us doing so, but I also wouldn't try to veto it, I think. Jan
On 05.07.19 09:59, Jan Beulich wrote: > On 04.07.2019 18:11, Paul Durrant wrote: >>> -----Original Message----- >>> From: Jan Beulich <JBeulich@suse.com> >>> Sent: 04 July 2019 16:49 >>> To: Anthony Perard <anthony.perard@citrix.com> >>> Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Konrad Rzeszutek Wilk >>> <konrad.wilk@oracle.com>; Juergen Gross <JGross@suse.com> >>> Subject: Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros >>> >>> On 04.07.2019 17:15, Anthony PERARD wrote: >>>> Those macros where introduced when a prefix "xen_" was added to >>>> mb,rmb,wmb. There are gated on __XEN_INTERFACE_VERSION__, but there >>>> are not part of the Xen interface. Users of ring.h needs to provide >>>> xen_[rw]?mb() anywai because [rw]?mb() isn't likely to exist. >>> >>> It's not clear to me what you want to achieve: >>> >> >> The issue is that any project importing this header (in this case QEMU, >> but I have the same issue in the Windows PV drivers) needs to import >> xen-compat.h (or dream up a header of the same name), even though this >> header is only concerned with the underpinnings of PV protocols and has >> nothing, as such, to do with Xen. > > While I agree this shouldn't have been part of the public headers, > that ship has sailed long, long ago. If a component doesn't use the > headers verbatim, I don't see why they couldn't remove that section > in their copy. If otoh they use the headers verbatim, then I'd > expect them to also use xen-compat.h Right. > >> To keep old verbatim users (are there really any at all?) happy, how about simple... >> >> #ifndef xen_mb() >> #define xen_mb() mb() >> #endif >> >> constructs? > > This would still cause conflicts if a component ends up defining > xen_mb() only after the inclusion of this header. As to there > actually being any - the old Linux 2.6.18 tree did pull in copies > of the headers without further editing. Beyond that while I'm > unaware of any, we simply can't know. Until now there simply was > an implied promise that we would try our best to avoid breaking > existing users. I'm completely on Jan's side here. What would be possible perhaps is to split ring.h into two headers: a new one with the pure ring definitions and ring.h #include-ing xen-compat.h and the new header and #define-ing the xen*mb() macros. Not sure this would be worth it, though. Juergen
> -----Original Message----- [snip] > > I'm completely on Jan's side here. > > What would be possible perhaps is to split ring.h into two headers: a > new one with the pure ring definitions and ring.h #include-ing > xen-compat.h and the new header and #define-ing the xen*mb() macros. > > Not sure this would be worth it, though. > Ok. Probably not worth it, as you say. If we don't feel comfortable remove old cruft like these then projects importing the headers will just have to hack it or live with importing xen-compat too. Anthony already submitted a patch doing the former for QEMU. Paul > > Juergen
On 05/07/2019 13:13, Paul Durrant wrote: >> -----Original Message----- > [snip] >> I'm completely on Jan's side here. >> >> What would be possible perhaps is to split ring.h into two headers: a >> new one with the pure ring definitions and ring.h #include-ing >> xen-compat.h and the new header and #define-ing the xen*mb() macros. >> >> Not sure this would be worth it, though. >> > Ok. Probably not worth it, as you say. If we don't feel comfortable remove old cruft like these then projects importing the headers will just have to hack it or live with importing xen-compat too. Anthony already submitted a patch doing the former for QEMU. Look. Either we expect people to copy the headers, or we expect to have a single canonical copy which is up to date and always backwards compatible. All documentation says "take a copy of the headers", and I have never seen anything, except the rather weird 2.6.18 driver port in tree, use the headers verbatim. These headers describe an ABI, not an API. Sure - the API is by convention but by the time people have taken a copy, they really are free to make modifications as they see fit, as long as they don't change the ABI. Insisting that everyone take a copy, *and* maintaining API compatibility for obsolete cruft is an exercise in self-flagilation. Given there are zero current consumers that we know of using the headers in a verbatim way, and all we're talking about is some pieces which really should never have been present in the first place, I think its entirely reasonable to make some changes and release note them. Xen 4.13 release: ... * Some obsolete warts in the public headers have been dropped. People syncing to this version should be aware of: 1) ... 2) ... Nothing here is rocket science, is it? ~Andrew
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h index c5d53e3103..5610ae14f8 100644 --- a/xen/include/public/io/ring.h +++ b/xen/include/public/io/ring.h @@ -33,6 +33,13 @@ * - standard integers types (uint8_t, uint16_t, etc) * They are provided by stdint.h of the standard headers. * + * Before using the different macros, you need to provide the following + * macros: + * - xen_mb() a memory barrier + * - xen_rmb() a read memory barrier + * - xen_wmb() a write memory barrier + * Example of those can be found in xenctrl.h. + * * In addition, if you intend to use the FLEX macros, you also need to * provide the following, before invoking the FLEX macros: * - size_t @@ -42,14 +49,6 @@ * and grant_table.h from the Xen public headers. */ -#include "../xen-compat.h" - -#if __XEN_INTERFACE_VERSION__ < 0x00030208 -#define xen_mb() mb() -#define xen_rmb() rmb() -#define xen_wmb() wmb() -#endif - typedef unsigned int RING_IDX; /* Round a 32-bit unsigned constant down to the nearest power of two. */
Those macros where introduced when a prefix "xen_" was added to mb,rmb,wmb. There are gated on __XEN_INTERFACE_VERSION__, but there are not part of the Xen interface. Users of ring.h needs to provide xen_[rw]?mb() anywai because [rw]?mb() isn't likely to exist. Suggested-by: Paul Durrant <Paul.Durrant@citrix.com> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- xen/include/public/io/ring.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)