include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

[Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Anthony PERARD 4 years, 9 months ago

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(-)

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. */
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Jan Beulich 4 years, 9 months ago

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Paul Durrant 4 years, 9 months ago

> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Jan Beulich 4 years, 9 months ago

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Juergen Gross 4 years, 9 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Paul Durrant 4 years, 9 months ago

> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] include/public/io/ring.h: Remove xen_mb, xen_rmb, xen_wmb macros

Posted by Andrew Cooper 4 years, 9 months ago

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel