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

Ilya Maximets i.maximets at ovn.org
Wed Mar 11 13:13:07 UTC 2020


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.


More information about the dev mailing list