diff mbox

[dpdk-dev,v3,11/29] eal: generic implementation for I/O device read/write access

Message ID 1484212646-10338-12-git-send-email-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Jerin Jacob Jan. 12, 2017, 9:17 a.m. UTC 
  This patch implements the generic version of rte_read[b/w/l/q]_[relaxed]
and rte_write[b/w/l/q]_[relaxed] using rte_io_wmb() and rte_io_rmb()

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 lib/librte_eal/common/include/generic/rte_io.h | 54 ++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

Comments

Thomas Monjalon Jan. 15, 2017, 9:29 p.m. UTC | #1 
2017-01-12 14:47, Jerin Jacob:
> +#define rte_read8_relaxed(addr) \
> +	({ uint8_t __v = *(const volatile uint8_t *)addr; __v; })

Why do you prefer a macro over an inline function?
It won't provide the same "debuggability".
Jerin Jacob Jan. 16, 2017, 3:26 a.m. UTC | #2 
On Sun, Jan 15, 2017 at 10:29:42PM +0100, Thomas Monjalon wrote:
> 2017-01-12 14:47, Jerin Jacob:
> > +#define rte_read8_relaxed(addr) \
> > +	({ uint8_t __v = *(const volatile uint8_t *)addr; __v; })
> 
> Why do you prefer a macro over an inline function?

In this case, I thought of avoiding any compiler behavior changes when
adding the new EAL APIs. Earlier, drivers were using direct pointer
dereference in code, I thought of using the macro to just substitute that to avoid
any performance regression due to this change for easy patchset acceptance.

IMO, One line macros are OK and for this specific case Linux also uses
readl/writel as macros.

Having said that, If you think it needs to be changed to "static inline", I
am fine with that. Let me know.

> It won't provide the same "debuggability".
Bruce Richardson Jan. 16, 2017, 11:01 a.m. UTC | #3 
On Mon, Jan 16, 2017 at 08:56:41AM +0530, Jerin Jacob wrote:
> On Sun, Jan 15, 2017 at 10:29:42PM +0100, Thomas Monjalon wrote:
> > 2017-01-12 14:47, Jerin Jacob:
> > > +#define rte_read8_relaxed(addr) \
> > > +	({ uint8_t __v = *(const volatile uint8_t *)addr; __v; })
> > 
> > Why do you prefer a macro over an inline function?
> 
> In this case, I thought of avoiding any compiler behavior changes when
> adding the new EAL APIs. Earlier, drivers were using direct pointer
> dereference in code, I thought of using the macro to just substitute that to avoid
> any performance regression due to this change for easy patchset acceptance.
> 
> IMO, One line macros are OK and for this specific case Linux also uses
> readl/writel as macros.
> 
> Having said that, If you think it needs to be changed to "static inline", I
> am fine with that. Let me know.
> 

My preference too is to go with static inline functions over macros
whenever possible.

/Bruce

> > It won't provide the same "debuggability".
Jerin Jacob Jan. 16, 2017, 11:17 a.m. UTC | #4 
On Mon, Jan 16, 2017 at 11:01:28AM +0000, Bruce Richardson wrote:
> On Mon, Jan 16, 2017 at 08:56:41AM +0530, Jerin Jacob wrote:
> > On Sun, Jan 15, 2017 at 10:29:42PM +0100, Thomas Monjalon wrote:
> > > 2017-01-12 14:47, Jerin Jacob:
> > > > +#define rte_read8_relaxed(addr) \
> > > > +	({ uint8_t __v = *(const volatile uint8_t *)addr; __v; })
> > > 
> > > Why do you prefer a macro over an inline function?
> > 
> > In this case, I thought of avoiding any compiler behavior changes when
> > adding the new EAL APIs. Earlier, drivers were using direct pointer
> > dereference in code, I thought of using the macro to just substitute that to avoid
> > any performance regression due to this change for easy patchset acceptance.
> > 
> > IMO, One line macros are OK and for this specific case Linux also uses
> > readl/writel as macros.
> > 
> > Having said that, If you think it needs to be changed to "static inline", I
> > am fine with that. Let me know.
> > 
> 
> My preference too is to go with static inline functions over macros
> whenever possible.

OK. I will change to static inline then

> 
> /Bruce
> 
> > > It won't provide the same "debuggability".
diff mbox

Patch

diff --git a/lib/librte_eal/common/include/generic/rte_io.h b/lib/librte_eal/common/include/generic/rte_io.h
index edfebf8..342bfec 100644
--- a/lib/librte_eal/common/include/generic/rte_io.h
+++ b/lib/librte_eal/common/include/generic/rte_io.h
@@ -34,6 +34,8 @@ 
 #ifndef _RTE_IO_H_
 #define _RTE_IO_H_
 
+#include <rte_atomic.h>
+
 /**
  * @file
  * I/O device memory operations
@@ -260,4 +262,56 @@  rte_write64(uint64_t value, volatile void *addr);
 
 #endif /* __DOXYGEN__ */
 
+#ifndef RTE_OVERRIDE_IO_H
+
+#define rte_read8_relaxed(addr) \
+	({ uint8_t __v = *(const volatile uint8_t *)addr; __v; })
+
+#define rte_read16_relaxed(addr) \
+	({ uint16_t __v = *(const volatile uint16_t *)addr; __v; })
+
+#define rte_read32_relaxed(addr) \
+	({ uint32_t __v = *(const volatile uint32_t *)addr; __v; })
+
+#define rte_read64_relaxed(addr) \
+	({ uint64_t __v = *(const volatile uint64_t *)addr; __v; })
+
+#define rte_write8_relaxed(value, addr) \
+	({ *(volatile uint8_t *)addr = value; })
+
+#define rte_write16_relaxed(value, addr) \
+	({ *(volatile uint16_t *)addr = value; })
+
+#define rte_write32_relaxed(value, addr) \
+	({ *(volatile uint32_t *)addr = value; })
+
+#define rte_write64_relaxed(value, addr) \
+	({ *(volatile uint64_t *)addr = value; })
+
+#define rte_read8(addr) \
+	({ uint8_t __v = *(const volatile uint8_t *)addr; rte_io_rmb(); __v; })
+
+#define rte_read16(addr) \
+	({uint16_t __v = *(const volatile uint16_t *)addr; rte_io_rmb(); __v; })
+
+#define rte_read32(addr) \
+	({uint32_t __v = *(const volatile uint32_t *)addr; rte_io_rmb(); __v; })
+
+#define rte_read64(addr) \
+	({uint64_t __v = *(const volatile uint64_t *)addr; rte_io_rmb(); __v; })
+
+#define rte_write8(value, addr) \
+	({ rte_io_wmb(); *(volatile uint8_t *)addr = value; })
+
+#define rte_write16(value, addr) \
+	({ rte_io_wmb(); *(volatile uint16_t *)addr = value; })
+
+#define rte_write32(value, addr) \
+	({ rte_io_wmb(); *(volatile uint32_t *)addr = value; })
+
+#define rte_write64(value, addr) \
+	({ rte_io_wmb(); *(volatile uint64_t *)addr = value; })
+
+#endif /* RTE_OVERRIDE_IO_H */
+
 #endif /* _RTE_IO_H_ */