Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Dumitrescu, Cristian
> Sent: Friday, January 11, 2019 2:40 AM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; dev@dpdk.org
> Cc: Pattan, Reshma <reshma.pat...@intel.com>; tho...@monjalon.net;
> olivier.m...@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct
> from function
>
>
>
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Thursday, January 10, 2019 6:07 PM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haa...@intel.com>; Pattan, Reshma
> > <reshma.pat...@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitre...@intel.com>; tho...@monjalon.net;
> > olivier.m...@6wind.com
> > Subject: [PATCH v2] mbuf: fix compile by removing struct from function
> >
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile.
> >
> > This fix removes the temporary struct rte_mbuf_sched, instead
> > reading from the mbuf directly for each struct member. As the
> > struct is now not used directly, the C++ compiler doesn't need
> > to know about the struct, resolving the issue.
> >
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> >
> > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
> >
> > ---
> >
> > Cc: reshma.pat...@intel.com
> > Cc: cristian.dumitre...@intel.com
> > Cc: tho...@monjalon.net
> > Cc: olivier.m...@6wind.com
> >
> > See mailing list for v1 discussion, perhaps this solution is more
> > readable due to leaving sched struct in-line in the mbuf struct.
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index bc562dc8a..1b260efd5 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf
> *m,
> > uint32_t *queue_id,
> >  uint8_t *traffic_class,
> >  uint8_t *color)
> >  {
> > -struct rte_mbuf_sched sched = m->hash.sched;

Did C++ complain *sched? *sched is better with less stack mem footprint.
If the pointer works, the code looks better.

> > -
> > -*queue_id = sched.queue_id;
> > -*traffic_class = sched.traffic_class;
> > -*color = sched.color;
> > +*queue_id = m->hash.sched.queue_id;
> > +*traffic_class = m->hash.sched.traffic_class;
> > +*color = m->hash.sched.color;
> >  }
> >
> >  /**
> > @@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m,
> > uint32_t queue_id,
> >  uint8_t traffic_class,
> >  uint8_t color)
> >  {
> > -m->hash.sched = (struct rte_mbuf_sched){
> > -.queue_id = queue_id,
> > -.traffic_class = traffic_class,
> > -.color = color,
> > -};
> > +m->hash.sched.queue_id = queue_id;
> > +m->hash.sched.traffic_class = traffic_class;
> > +m->hash.sched.color = color;
> >  }
> >
> >  #ifdef __cplusplus
> > --
> > 2.17.1
>
> NAK.
>
> I am fine with V1, but against this V2 due to the reasons previously discussed
> and agreed by Olivier [1] regarding performance. We should not sacrifice
> performance for the sake of cosmetics criteria that can met some other way.
>
> In order to meet readability requirements from Olivier, I suggest we go back
> to V1 and we explicitly mention the size of the mbuf->sched field inslide the
> mbuf as 8 bytes:
>
> struct rte_mbuf {
> ...
> struct rte_mbuf_sched sched; /**< Hierarchical scheduler: 8 bytes
> */
> ...
> }
>
> Olivier, is this a good compromise?
>
> Regards,
> Cristian
>
> [1] https://mails.dpdk.org/archives/dev/2018-December/121806.html

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.