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

[Flavio Leitner]

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.

-- 
fbl