[ovs-dev] [PATCHv7 1/2] userspace: Enable TSO support for non-DPDK.

William Tu u9012063 at gmail.com
Thu Mar 19 23:47:24 UTC 2020


On Thu, Mar 19, 2020 at 2:14 PM Flavio Leitner <fbl at sysclose.org> wrote:
>
>
> Hi William,
>
> Nice that the amount of specific DPDK and non-DPDK reduced a lot!
> I haven't tried to build or test yet.
>
> It may be nitpicking because you didn't introduce some of the
> names in this patch so I understand it is not the goal of your
> patch. However, I think it can take the opportunity to make it
> clear and uniform.  See below.
>

Hi Flavio,
No problem, thanks for your review.

> On Tue, Mar 17, 2020 at 01:02:08PM -0700, William Tu wrote:
> [...]
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -46,21 +46,58 @@ enum OVS_PACKED_ENUM dp_packet_source {
> >      DPBUF_AFXDP,               /* Buffer data from XDP frame. */
> >  };
> >
> > -#define DP_PACKET_CONTEXT_SIZE 64
> > -
> > -#ifndef DPDK_NETDEV
> > -/* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
> > -enum dp_packet_offload_mask {
> > -    DP_PACKET_OL_RSS_HASH_MASK  = 0x1, /* Is the 'rss_hash' valid? */
> > -    DP_PACKET_OL_FLOW_MARK_MASK = 0x2, /* Is the 'flow_mark' valid? */
> > -};
> > -#else
> > +#ifdef DPDK_NETDEV
> >  /* DPDK mbuf ol_flags that are not really an offload flags.  These are mostly
> >   * related to mbuf memory layout and OVS should not touch/clear them. */
> >  #define DPDK_MBUF_NON_OFFLOADING_FLAGS (EXT_ATTACHED_MBUF | \
> >                                          IND_ATTACHED_MBUF)
>
> That should end with _MASK (multiple bits) as others.

OK.

>
> >  #endif
> >
> > +#define DP_PACKET_CONTEXT_SIZE 64
> > +#ifdef DPDK_NETDEV
> > +#define DEF_OL_FLAG(NAME, DPDK_DEF, GENERIC_DEF) NAME = DPDK_DEF
> > +#else
> > +#define DEF_OL_FLAG(NAME, DPDK_DEF, GENERIC_DEF) NAME = GENERIC_DEF
> > +#endif
> > +
> > +/* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
> > +enum dp_packet_offload_mask {
> > +    /* Reset offload. */
> > +    DEF_OL_FLAG(DP_PACKET_OL_NONE, DPDK_MBUF_NON_OFFLOADING_FLAGS, 0x0),
>
> Same issue here: I think it could be DP_PACKET_OL_NONE_MASK, otherwise it
> is easy to check as a bit (var & DP_PACKET_OL_NONE) which will return true
> even though the bit mask would not be same, for example:
> ((var & DP_PACKET_OL_MASK) == DP_PACKET_OL_MASK)

Yes.
>
> > +    /* Is the 'rss_hash' valid? */
> > +    DEF_OL_FLAG(DP_PACKET_OL_RSS_HASH_MASK, PKT_RX_RSS_HASH, 0x1),
>
> The RSS above is a bit and not a MASK, so the name is misleading.
OK
>
> > +    /* Is the 'flow_mark' valid? (DPDK does not support) */
> > +    DEF_OL_FLAG(DP_PACKET_OL_FLOW_MARK_MASK, PKT_RX_FDIR_ID, 0x2),
>
> Here as well, a misleading name.
> What do you think?
OK I will remove the _MASK here also.
Thanks
William


More information about the dev mailing list