[ovs-dev] [PATCHv3 1/2] userspace: Enable TSO support for non-DPDK.
Flavio Leitner
fbl at sysclose.org
Wed Mar 11 12:13:55 UTC 2020
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.
> Defines of flags might be shortened this way:
>
> #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
>
> enum dp_packet_offload_mask {
> ...
> /* Valid IP checksum in the packet. */
> DEF_OL_FLAG(DP_PACKET_OL_RX_IP_CKSUM_GOOD, PKT_RX_IP_CKSUM_GOOD, 0x20),
> /* TCP Segmentation Offload. */
> DEF_OL_FLAG(DP_PACKET_OL_TX_TCP_SEG, PKT_TX_TCP_SEG, 0x40),
> ...
> };
Exactly, looks better that way.
Thanks,
--
fbl
More information about the dev
mailing list