[ovs-dev] [PATCH v7 3/9] dp-packet: Refactor offloading API.

[Ian Stokes]

On 3/13/2019 10:09 AM, Ian Stokes wrote:
> On 2/26/2019 10:38 AM, Ilya Maximets wrote:
>> 1. No reason to have mbuf related APIs in a generic code.
>> 2. Not only RSS/checksums should be invalidated in case of tunnel
>>     decapsulation or sending to 'ring' ports.
>>
>> In order to fix two above issues, new function
>> 'dp_packet_reset_offload' introduced. In order to clean up/unify
>> the code and simplify addition of new offloading features to non-DPDK
>> version of dp_packet, introduced 'ol_flags' bitmask. Additionally
>> reduced code complexity in 'dp_packet_clone_with_headroom' by using
>> already existent generic APIs.
>>  > Unfortunately, we still need to have a special case for mbuf
>> initialization inside 'dp_packet_init__()'.
>> 'dp_packet_init_specific()' introduced for this purpose as a generic
>> API for initialization of the implementation-specific fields.
>>
> 
> Thanks for this Ilya, apologies for the delay with the series, should 
> have more time to look at it this week.
> 
> One minor comment below, but other than that seems ok.
> 
>> Acked-by: Flavio Leitner <fbl at sysclose.org>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>   lib/dp-packet.c   | 15 ++++------
>>   lib/dp-packet.h   | 75 ++++++++++++++++++++---------------------------
>>   lib/netdev-dpdk.c |  6 ++--
>>   lib/netdev.c      |  4 +--
>>   4 files changed, 41 insertions(+), 59 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 93b0e9c84..f8207ffc2 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -30,9 +30,10 @@ dp_packet_init__(struct dp_packet *b, size_t 
>> allocated, enum dp_packet_source so
>>       b->source = source;
>>       dp_packet_reset_offsets(b);
>>       pkt_metadata_init(&b->md, 0);
>> -    dp_packet_rss_invalidate(b);
>> -    dp_packet_mbuf_init(b);
>>       dp_packet_reset_cutlen(b);
>> +    dp_packet_reset_offload(b);
>> +    /* Initialize implementation-specific fields of dp_packet. */
>> +    dp_packet_init_specific(b);
>>       /* By default assume the packet type to be Ethernet. */
>>       b->packet_type = htonl(PT_ETH);
>>   }
>> @@ -173,16 +174,10 @@ dp_packet_clone_with_headroom(const struct 
>> dp_packet *buffer, size_t headroom)
>>   #ifdef DPDK_NETDEV
>>       new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> -#else
>> -    new_buffer->rss_hash_valid = buffer->rss_hash_valid;
>>   #endif
>> -    if (dp_packet_rss_valid(new_buffer)) {
>> -#ifdef DPDK_NETDEV
>> -        new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
>> -#else
>> -        new_buffer->rss_hash = buffer->rss_hash;
>> -#endif
>> +    if (dp_packet_rss_valid(buffer)) {
>> +        dp_packet_set_rss_hash(new_buffer, 
>> dp_packet_get_rss_hash(buffer));
>>       }
>>       return new_buffer;
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index c6672f6be..b34dada78 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -46,6 +46,13 @@ enum OVS_PACKED_ENUM dp_packet_source {
>>   #define DP_PACKET_CONTEXT_SIZE 64
>> +#ifndef DPDK_NETDEV
>> +/* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
>> +enum dp_packet_offload_mask {
>> +    DP_PACKET_OL_RSS_HASH_MASK  = 0x1, /* Is the 'rss_hash' valid? */
>> +};
> 
> Is there a specific reason you've used an enum instead of a define here 
> as its just a single value.
> 
> Is the expectation that the entries for the offload mask will be 
> expanded in the future to be more than just RSS_HASH_MASK?
> 

Ah, please disregard, I've just spotted that it is expanded later in the 
series.

I've applied this to master.

Thanks
Ian