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

Flavio Leitner fbl at sysclose.org
Wed Mar 11 20:22:25 UTC 2020


On Wed, Mar 11, 2020 at 02:13:07PM +0100, Ilya Maximets wrote:
> On 3/11/20 1:37 PM, Flavio Leitner wrote:
> > On Wed, Mar 11, 2020 at 01:30:32PM +0100, Ilya Maximets wrote:
> >> On 3/11/20 1:13 PM, Flavio Leitner wrote:
> >>> On Wed, Mar 11, 2020 at 11:47:22AM +0100, Ilya Maximets wrote:
> >>>> On 3/10/20 10:01 PM, Flavio Leitner wrote:
> >>>>>
> >>>>> Hi William,
> >>>>>
> >>>>> Any chance to get the function comments copied to the non-dpdk
> >>>>> versions as well?
> >>>>>
> >>>>> I wonder if we could define DP_PACKET_OL.* defines as it is
> >>>>> proposed in this patch if DPDK is not enabled, or define them
> >>>>> as DPDK defines. For example:
> >>>>>
> >>>>> #ifdef DPDK
> >>>>>     DP_PACKET_OL_TX_TCP_SEG = PKT_TX_TCP_SEG
> >>>>> #else
> >>>>>     DP_PACKET_OL_TX_TCP_SEG = 0x40
> >>>>> #endif
> >>>>>
> >>>>> Then
> >>>>> struct dp_packet
> >>>>> #ifdef DPDK_NETDEV
> >>>>>     struct rte_mbuf mbuf;       /* DPDK mbuf */
> >>>>> #define ol_flags mbuf.ol_flags
> >>>>> #else
> >>>>>     void *base_;                /* First byte of allocated space. */
> >>>>>     uint16_t allocated_;        /* Number of bytes allocated. */
> >>>>>     uint16_t data_ofs;          /* First byte actually in use. */
> >>>>>     uint32_t size_;             /* Number of bytes in use. */
> >>>>>     uint32_t ol_flags;          /* Offloading flags. */
> >>>>>
> >>>>>
> >>>>>
> >>>>> Then we would have a single:
> >>>>> static inline bool
> >>>>> dp_packet_hwol_is_tso(const struct dp_packet *b)
> >>>>> {
> >>>>>     return !!(b->ol_flags & PKT_TX_TCP_SEG);
> >>>>> }
> >>>>>
> >>>>> I haven't tried, so don't know if I am missing something or if it
> >>>>> will look ugly, but it would save us many function copies.
> >>>>
> >>>>
> >>>> Interesting.  I generally like the idea except the ' #define ol_flags'
> >>>> part.   We should define something that could not be used accidentally,
> >>>> e.g.
> >>>>
> >>>> #ifdef DPDK_NETDEV
> >>>> #define DP_PACKET_OL_FLAGS_FIELD  mbuf.ol_flags
> >>>> #else
> >>>> #define DP_PACKET_OL_FLAGS_FIELD  ol_flags
> >>>> #endif
> >>>> ...
> >>>>     return !!(b->DP_PACKET_OL_FLAGS_FIELD & PKT_TX_TCP_SEG);
> >>>>
> >>>>
> >>>> Or, better write the inline access function that should look much
> >>>> more clean:
> >>>>
> >>>> static inline uint32_t *
> >>>> dp_packet_ol_flags_get(const struct dp_packet *b)
> >>>> {
> >>>> #ifdef DPDK_NETDEV
> >>>>     return &b->mbuf.ol_flags;
> >>>> #else
> >>>>     return &b->ol_flags;
> >>>> #endif
> >>>> }
> >>>> ...
> >>>>     return !!(*dp_packet_ol_flags_get(b) & PKT_TX_TCP_SEG);
> >>>> ...
> >>>>     *dp_packet_ol_flags_get(b) |= PKT_TX_SCTP_CKSUM;
> >>>> ...
> >>>>
> >>>>
> >>>> What do you think?
> >>>
> >>> Then we will have cases like this just to set another flag:
> >>>     dp_packet_ol_flags_set(b, dp_packet_ol_flags_get(b) | FLAG);
> >>>
> >>> I suspect that the define you proposed is better or the original
> >>> one.
> >>
> >> I didn't propose the _set() function.   See example of flag setting
> >> few lines above.
> > 
> > I meant that we would need a 'set' either by a function/define or
> > other unusual way. IMO, '*dp_packet_ol_flags_get(b) |= ' isn't usual.
> 
> I don't insist. Just a note that such constructions are not very unusual.
> In OVS coverage counters and per-thread static data implemented this way.
> There are a lot of examples in Linux kernel, e.g. per_cpu_ptr().
> '_get()' might be replaced with '_ptr()' for readability, though.

Definitely the _ptr() makes a difference.

Another approach is using union:
struct dp_packet {
#ifdef DPDK
    struct mbuf mbuf;
#else
    ...
    union {
        int ol_flags;
    } mbuf;
    ...
#endif
};

Then we always have:
    packet->mbuf.ol_flags = 0x10;

I don't have a strong opinion.
Thanks,
-- 
fbl


More information about the dev mailing list