mbuf: fix compile by making sched struct visible
Checks
Commit Message
Although C compilation works with the struct rte_mbuf_sched
declared inside the struct rte_mbuf namespace, C++ fails to
compile. This fix moves the rte_mbuf_sched struct up to the
global namespace, instead of declaring it inside the struct
mbuf namespace.
The struct rte_mbuf_sched is being used on the stack in
rte_mbuf_sched_get() and as a cast in _set(). For this
reason, it must be exposed as an available type.
Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
Cc: reshma.pattan@intel.com
Cc: cristian.dumitrescu@intel.com
Cc: thomas@monjalon.net
Hey folks,
Currently the mbuf header will fail to compile with a C++ compiler,
this patch is one possible solution. I'm not particularly happy with
this as a fix as it reduces mbuf struct readability, however it does
resolve the issue.
Regards, -Harry
---
lib/librte_mbuf/rte_mbuf.h | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
Comments
10/01/2019 17:50, Harry van Haaren:
> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile. This fix moves the rte_mbuf_sched struct up to the
> global namespace, instead of declaring it inside the struct
> mbuf namespace.
>
> The struct rte_mbuf_sched is being used on the stack in
> rte_mbuf_sched_get() and as a cast in _set(). For this
> reason, it must be exposed as an available type.
>
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> ---
>
> Cc: reshma.pattan@intel.com
> Cc: cristian.dumitrescu@intel.com
> Cc: thomas@monjalon.net
>
> Hey folks,
>
> Currently the mbuf header will fail to compile with a C++ compiler,
> this patch is one possible solution. I'm not particularly happy with
> this as a fix as it reduces mbuf struct readability, however it does
> resolve the issue.
What are the other possible solutions?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, January 10, 2019 5:35 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Dumitrescu,
> Cristian <cristian.dumitrescu@intel.com>; olivier.matz@6wind.com
> Subject: Re: [PATCH] mbuf: fix compile by making sched struct visible
>
> 10/01/2019 17:50, Harry van Haaren:
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile. This fix moves the rte_mbuf_sched struct up to the
> > global namespace, instead of declaring it inside the struct
> > mbuf namespace.
> >
> > The struct rte_mbuf_sched is being used on the stack in
> > rte_mbuf_sched_get() and as a cast in _set(). For this
> > reason, it must be exposed as an available type.
> >
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > Cc: reshma.pattan@intel.com
> > Cc: cristian.dumitrescu@intel.com
> > Cc: thomas@monjalon.net
> >
> > Hey folks,
> >
> > Currently the mbuf header will fail to compile with a C++ compiler,
> > this patch is one possible solution. I'm not particularly happy with
> > this as a fix as it reduces mbuf struct readability, however it does
> > resolve the issue.
>
> What are the other possible solutions?
I guess we could avoid using the struct in inline functions, by reading
or writing from the mbuf struct directly (without pulling out a temporary
rte_mbuf_sched field):
get()
*queue_id = m->hash.sched.queue_id;
...
set()
m->hash.sched.queue_id = queue_id;
...
I believe this was discussed when the patch was being reviewed;
http://patches.dpdk.org/patch/49126/
I have a mild preference to keep all mbuf structs visible inside the unions,
it makes it easier to understand the layout of mbuf, hence I prefer the
above pseudo code suggestion over the v1 patch sent before.
If others have a strong opinion for another solution, I'm ok with it.
On Thu, 10 Jan 2019 16:50:51 +0000
Harry van Haaren <harry.van.haaren@intel.com> wrote:
> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile. This fix moves the rte_mbuf_sched struct up to the
> global namespace, instead of declaring it inside the struct
> mbuf namespace.
>
> The struct rte_mbuf_sched is being used on the stack in
> rte_mbuf_sched_get() and as a cast in _set(). For this
> reason, it must be exposed as an available type.
>
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
I believe this was done so that the compiler doesn't generate
bad code.
If you reference the mbuf to get the fields then each operation becomes
a load shift and mask operation to get to the bitfield. But if they
are local then this is all done on a single register value.
Check the generated code.
One solution would be to move this into a private header file
where C++ won't find it.
@@ -468,6 +468,17 @@ __extension__
typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
* with a single assignment */
+struct rte_mbuf_sched {
+ uint32_t queue_id; /**< Queue ID. */
+ uint8_t traffic_class;
+ /**< Traffic class ID. Traffic class 0
+ * is the highest priority traffic class.
+ */
+ uint8_t color;
+ /**< Color. @see enum rte_color.*/
+ uint16_t reserved; /**< Reserved. */
+}; /**< Hierarchical scheduler */
+
/**
* The generic rte_mbuf, containing a packet mbuf.
*/
@@ -574,16 +585,7 @@ struct rte_mbuf {
* on PKT_RX_FDIR_* flag in ol_flags.
*/
} fdir; /**< Filter identifier if FDIR enabled */
- struct rte_mbuf_sched {
- uint32_t queue_id; /**< Queue ID. */
- uint8_t traffic_class;
- /**< Traffic class ID. Traffic class 0
- * is the highest priority traffic class.
- */
- uint8_t color;
- /**< Color. @see enum rte_color.*/
- uint16_t reserved; /**< Reserved. */
- } sched; /**< Hierarchical scheduler */
+ struct rte_mbuf_sched sched; /**< Hierarchical scheduler */
struct {
uint32_t reserved1;
uint16_t reserved2;