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

Ilya Maximets i.maximets at ovn.org
Wed Mar 11 12:30:32 UTC 2020


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.

>     
> 
>> 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,
> 



More information about the dev mailing list