[ovs-dev] [PATCH v2 2/8] dpif-netdev: retrieve flow directly from the flow mark

[Simon Horman]

On Mon, Sep 11, 2017 at 01:37:10PM +0800, Yuanhan Liu wrote:
> On Fri, Sep 08, 2017 at 05:38:07PM +0000, Darrell Ball wrote:
> >     > +static inline bool
> >     > +dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED,
> >     > +                        uint32_t *mark OVS_UNUSED)
> >     > +{
> >     > +#ifdef DPDK_NETDEV
> >     > +    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
> >     > +        *mark = p->mbuf.hash.fdir.hi;
> >     > +        return true;
> >     > +    }
> >     > +#endif
> >     > +    return false;
> >     > +}
> >     
> >     It seems odd that p and mark are marked as OVS_UNUSED
> >     but used in the case that DPDK_NETDEV is defined.
> > 
> > [Darrell] OVS_UNUSED means ‘’may be unused”
> >                 Typically, for a trivial alternative, we would do a slight variation of the above
> >                 rather than writing two versions of the functions.
> 
> Right. I also saw quite many examples like this in OVS. That's why I
> code in this way, though I agree with Simon, it's indeed a bit odd.

Thanks. In light of the above I have no objections to
dp_packet_has_flow_mark() the way it is.

> >     > +    if (OVS_LIKELY(!(nw_frag & FLOW_NW_FRAG_LATER))) {
> >     > +        if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
> >     > +            if (OVS_LIKELY(size >= TCP_HEADER_LEN)) {
> >     
> >     The following might be nicer:
> >     
> >     +    if (OVS_LIKELY(!(nw_frag & FLOW_NW_FRAG_LATER))
> >     +        && OVS_LIKELY(nw_proto == IPPROTO_TCP)
> >     +        && OVS_LIKELY(size >= TCP_HEADER_LEN)) {
> 
> Indeed. Thanks!
> 
> 	--yliu