| Message ID | 1468037577-6527-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/07/16 05:12, Corneliu ZUZU wrote: > This wouldn't let me make a param of a function that used atomic_read() const. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> This is a good improvement, but you must make an identical adjustment to the arm code, otherwise you will end up with subtle build failures. If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew > --- > xen/include/asm-x86/atomic.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h > index d246b70..0b250c8 100644 > --- a/xen/include/asm-x86/atomic.h > +++ b/xen/include/asm-x86/atomic.h > @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t; > * > * Atomically reads the value of @v. > */ > -static inline int atomic_read(atomic_t *v) > +static inline int atomic_read(const atomic_t *v) > { > return read_atomic(&v->counter); > } > @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v) > * > * Non-atomically reads the value of @v > */ > -static inline int _atomic_read(atomic_t v) > +static inline int _atomic_read(const atomic_t v) > { > return v.counter; > }
Hi Andrew, On 7/11/2016 6:18 PM, Andrew Cooper wrote: > On 09/07/16 05:12, Corneliu ZUZU wrote: >> This wouldn't let me make a param of a function that used atomic_read() const. >> >> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > This is a good improvement, but you must make an identical adjustment to > the arm code, otherwise you will end up with subtle build failures. Right, didn't even realize it was X86-specific. > > If you are really feeling up to it, having a common xen/atomic.h with > > typedef struct { int counter; } atomic_t; > #define ATOMIC_INIT(i) { (i) } > > and some prototypes such as: > > static inline int atomic_read(const atomic_t *v); > > would be great, but this looks like it has the possibility to turn into > a rats nest. If it does, then just doubling up this code for arm is ok. > > ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. It might be better to keep them separate - they don't add much anyway since their implementation is short - than risk unexpected different behavior on a future arch. But then again I don't know much details of their implementation, so anyway, I'd surely prefer to do this kind of change in a separate patch. > >> --- >> xen/include/asm-x86/atomic.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h >> index d246b70..0b250c8 100644 >> --- a/xen/include/asm-x86/atomic.h >> +++ b/xen/include/asm-x86/atomic.h >> @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t; >> * >> * Atomically reads the value of @v. >> */ >> -static inline int atomic_read(atomic_t *v) >> +static inline int atomic_read(const atomic_t *v) >> { >> return read_atomic(&v->counter); >> } >> @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v) >> * >> * Non-atomically reads the value of @v >> */ >> -static inline int _atomic_read(atomic_t v) >> +static inline int _atomic_read(const atomic_t v) >> { >> return v.counter; >> } > Thanks, Zuzu C.
On 12/07/16 06:11, Corneliu ZUZU wrote: > Hi Andrew, > > On 7/11/2016 6:18 PM, Andrew Cooper wrote: >> On 09/07/16 05:12, Corneliu ZUZU wrote: >>> This wouldn't let me make a param of a function that used >>> atomic_read() const. >>> >>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >> This is a good improvement, but you must make an identical adjustment to >> the arm code, otherwise you will end up with subtle build failures. > > Right, didn't even realize it was X86-specific. It isn't x86 specific. (Which is what I presume you meant to say.) > >> >> If you are really feeling up to it, having a common xen/atomic.h with >> >> typedef struct { int counter; } atomic_t; >> #define ATOMIC_INIT(i) { (i) } >> >> and some prototypes such as: >> >> static inline int atomic_read(const atomic_t *v); >> >> would be great, but this looks like it has the possibility to turn into >> a rats nest. If it does, then just doubling up this code for arm is ok. >> >> ~Andrew > > Yes, that might be more complicated than we expect and I don't know if > making code such as this common would be a good idea, usually these > functions are always architecture-specific. I only suggested making the prototype common, not the implementation. As such, the issue you accidentally introduced would become a hard build failure on affected architectures, rather than a subtle build failure in common code at some point in the future. ~Andrew
On 7/12/2016 12:42 PM, Andrew Cooper wrote: > On 12/07/16 06:11, Corneliu ZUZU wrote: >> Hi Andrew, >> >> On 7/11/2016 6:18 PM, Andrew Cooper wrote: >>> On 09/07/16 05:12, Corneliu ZUZU wrote: >>>> This wouldn't let me make a param of a function that used >>>> atomic_read() const. >>>> >>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> >>> This is a good improvement, but you must make an identical adjustment to >>> the arm code, otherwise you will end up with subtle build failures. >> Right, didn't even realize it was X86-specific. > It isn't x86 specific. (Which is what I presume you meant to say.) Heh, yes, depends how you look at it, I was referring precisely to (the implementation of) the function I've modified (which was X86-specific, called from common, which meant there's an ARM one as well). > >>> If you are really feeling up to it, having a common xen/atomic.h with >>> >>> typedef struct { int counter; } atomic_t; >>> #define ATOMIC_INIT(i) { (i) } >>> >>> and some prototypes such as: >>> >>> static inline int atomic_read(const atomic_t *v); >>> >>> would be great, but this looks like it has the possibility to turn into >>> a rats nest. If it does, then just doubling up this code for arm is ok. >>> >>> ~Andrew >> Yes, that might be more complicated than we expect and I don't know if >> making code such as this common would be a good idea, usually these >> functions are always architecture-specific. > I only suggested making the prototype common, not the implementation. > As such, the issue you accidentally introduced would become a hard build > failure on affected architectures, rather than a subtle build failure in > common code at some point in the future. > > ~Andrew > Oh, I see, good idea, I've just tested it and it works, what did you have in mind when you said it could cause problems? Zuzu C.
On 12/07/16 11:11, Corneliu ZUZU wrote: > >> >>>> If you are really feeling up to it, having a common xen/atomic.h with >>>> >>>> typedef struct { int counter; } atomic_t; >>>> #define ATOMIC_INIT(i) { (i) } >>>> >>>> and some prototypes such as: >>>> >>>> static inline int atomic_read(const atomic_t *v); >>>> >>>> would be great, but this looks like it has the possibility to turn >>>> into >>>> a rats nest. If it does, then just doubling up this code for arm >>>> is ok. >>>> >>>> ~Andrew >>> Yes, that might be more complicated than we expect and I don't know if >>> making code such as this common would be a good idea, usually these >>> functions are always architecture-specific. >> I only suggested making the prototype common, not the implementation. >> As such, the issue you accidentally introduced would become a hard build >> failure on affected architectures, rather than a subtle build failure in >> common code at some point in the future. >> >> ~Andrew >> > > Oh, I see, good idea, I've just tested it and it works, what did you > have in mind when you said it could cause problems? The build issues would come at some point later when someone attempts to atomic_read() a constant atomic_t in common code, when the ARM build would break. ~Andrew
On 7/12/2016 1:22 PM, Andrew Cooper wrote: > On 12/07/16 11:11, Corneliu ZUZU wrote: >>>>> If you are really feeling up to it, having a common xen/atomic.h with >>>>> >>>>> typedef struct { int counter; } atomic_t; >>>>> #define ATOMIC_INIT(i) { (i) } >>>>> >>>>> and some prototypes such as: >>>>> >>>>> static inline int atomic_read(const atomic_t *v); >>>>> >>>>> would be great, but this looks like it has the possibility to turn >>>>> into >>>>> a rats nest. If it does, then just doubling up this code for arm >>>>> is ok. >>>>> >>>>> ~Andrew >>>> Yes, that might be more complicated than we expect and I don't know if >>>> making code such as this common would be a good idea, usually these >>>> functions are always architecture-specific. >>> I only suggested making the prototype common, not the implementation. >>> As such, the issue you accidentally introduced would become a hard build >>> failure on affected architectures, rather than a subtle build failure in >>> common code at some point in the future. >>> >>> ~Andrew >>> >> Oh, I see, good idea, I've just tested it and it works, what did you >> have in mind when you said it could cause problems? > The build issues would come at some point later when someone attempts to > atomic_read() a constant atomic_t in common code, when the ARM build > would break. > > ~Andrew Sorry, could you rephrase this? When the ARM build would break (i.e. fail, I presume) because of what? Zuzu C.
On 7/12/2016 1:22 PM, Andrew Cooper wrote: > On 12/07/16 11:11, Corneliu ZUZU wrote: >>>>> If you are really feeling up to it, having a common xen/atomic.h with >>>>> >>>>> typedef struct { int counter; } atomic_t; >>>>> #define ATOMIC_INIT(i) { (i) } >>>>> >>>>> and some prototypes such as: >>>>> >>>>> static inline int atomic_read(const atomic_t *v); >>>>> >>>>> would be great, but this looks like it has the possibility to turn >>>>> into >>>>> a rats nest. If it does, then just doubling up this code for arm >>>>> is ok. >>>>> >>>>> ~Andrew >>>> Yes, that might be more complicated than we expect and I don't know if >>>> making code such as this common would be a good idea, usually these >>>> functions are always architecture-specific. >>> I only suggested making the prototype common, not the implementation. >>> As such, the issue you accidentally introduced would become a hard build >>> failure on affected architectures, rather than a subtle build failure in >>> common code at some point in the future. >>> >>> ~Andrew >>> >> Oh, I see, good idea, I've just tested it and it works, what did you >> have in mind when you said it could cause problems? > The build issues would come at some point later when someone attempts to > atomic_read() a constant atomic_t in common code, when the ARM build > would break. > > ~Andrew Ooh, no, I was asking what you meant when you said "this looks like it has the possibility to turn into a rats nest" in your first message, not the thing about hard build failure.. Zuzu C.
On 12/07/16 11:38, Corneliu ZUZU wrote: > On 7/12/2016 1:22 PM, Andrew Cooper wrote: >> On 12/07/16 11:11, Corneliu ZUZU wrote: >>>>>> If you are really feeling up to it, having a common xen/atomic.h >>>>>> with >>>>>> >>>>>> typedef struct { int counter; } atomic_t; >>>>>> #define ATOMIC_INIT(i) { (i) } >>>>>> >>>>>> and some prototypes such as: >>>>>> >>>>>> static inline int atomic_read(const atomic_t *v); >>>>>> >>>>>> would be great, but this looks like it has the possibility to turn >>>>>> into >>>>>> a rats nest. If it does, then just doubling up this code for arm >>>>>> is ok. >>>>>> >>>>>> ~Andrew >>>>> Yes, that might be more complicated than we expect and I don't >>>>> know if >>>>> making code such as this common would be a good idea, usually these >>>>> functions are always architecture-specific. >>>> I only suggested making the prototype common, not the implementation. >>>> As such, the issue you accidentally introduced would become a hard >>>> build >>>> failure on affected architectures, rather than a subtle build >>>> failure in >>>> common code at some point in the future. >>>> >>>> ~Andrew >>>> >>> Oh, I see, good idea, I've just tested it and it works, what did you >>> have in mind when you said it could cause problems? >> The build issues would come at some point later when someone attempts to >> atomic_read() a constant atomic_t in common code, when the ARM build >> would break. >> >> ~Andrew > > Ooh, no, I was asking what you meant when you said "this looks like it > has the possibility to turn into a rats nest" in your first message, > not the thing about hard build failure.. Ah. sorry. You would have to invert all the includes of atomic.h to include <xen/atomic.h> rather than <asm/atomic.h>, and have xen/atomic.h include asm/atomic.h towards the end, such that the common prototypes are first. I just suspect that this might not be completely trivial to untangle (of course, I could also be wrong). ~Andrew
On 7/12/2016 3:49 PM, Andrew Cooper wrote: > On 12/07/16 11:38, Corneliu ZUZU wrote: >> On 7/12/2016 1:22 PM, Andrew Cooper wrote: >>> On 12/07/16 11:11, Corneliu ZUZU wrote: >>>>>>> If you are really feeling up to it, having a common xen/atomic.h >>>>>>> with >>>>>>> >>>>>>> typedef struct { int counter; } atomic_t; >>>>>>> #define ATOMIC_INIT(i) { (i) } >>>>>>> >>>>>>> and some prototypes such as: >>>>>>> >>>>>>> static inline int atomic_read(const atomic_t *v); >>>>>>> >>>>>>> would be great, but this looks like it has the possibility to turn >>>>>>> into >>>>>>> a rats nest. If it does, then just doubling up this code for arm >>>>>>> is ok. >>>>>>> >>>>>>> ~Andrew >>>>>> Yes, that might be more complicated than we expect and I don't >>>>>> know if >>>>>> making code such as this common would be a good idea, usually these >>>>>> functions are always architecture-specific. >>>>> I only suggested making the prototype common, not the implementation. >>>>> As such, the issue you accidentally introduced would become a hard >>>>> build >>>>> failure on affected architectures, rather than a subtle build >>>>> failure in >>>>> common code at some point in the future. >>>>> >>>>> ~Andrew >>>>> >>>> Oh, I see, good idea, I've just tested it and it works, what did you >>>> have in mind when you said it could cause problems? >>> The build issues would come at some point later when someone attempts to >>> atomic_read() a constant atomic_t in common code, when the ARM build >>> would break. >>> >>> ~Andrew >> Ooh, no, I was asking what you meant when you said "this looks like it >> has the possibility to turn into a rats nest" in your first message, >> not the thing about hard build failure.. > Ah. sorry. > > You would have to invert all the includes of atomic.h to include > <xen/atomic.h> rather than <asm/atomic.h>, and have xen/atomic.h include > asm/atomic.h towards the end, such that the common prototypes are > first. I just suspect that this might not be completely trivial to > untangle (of course, I could also be wrong). > > ~Andrew I did it the other way - included <xen/monitor.h> from <asm/monitor.h>. Zuzu C.
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..0b250c8 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(&v->counter); } @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v) * * Non-atomically reads the value of @v */ -static inline int _atomic_read(atomic_t v) +static inline int _atomic_read(const atomic_t v) { return v.counter; }
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/include/asm-x86/atomic.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)