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

William Tu u9012063 at gmail.com
Wed Mar 11 22:04:03 UTC 2020


On Wed, Mar 11, 2020 at 1:22 PM Flavio Leitner <fbl at sysclose.org> wrote:
>
> 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;

This might mislead people that DPDK mbuf is being used.

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

 Hi Flavio and Ilya,

Thanks a lot for these valuable feedbacks.
I will work on the next version.

William


More information about the dev mailing list