[ovs-dev] [PATCH v3 1/2] dpctl: dpif: add kernel datapath cache hit output

[Eelco Chaudron]

On 3 Mar 2021, at 21:44, Flavio Leitner wrote:

> On Wed, Mar 03, 2021 at 02:52:33PM +0100, Eelco Chaudron wrote:
>> This patch adds cache usage statistics to the output:
>>
>> $ ovs-dpctl show
>> system at ovs-system:
>>   lookups: hit:24 missed:71 lost:0
>>   flows: 0
>>   masks: hit:334 total:0 hit/pkt:3.52
>>   cache: hit:4 hit rate:4.2105%
>
> nit: maybe the maintainer can fix that to be 2 digits as the code 
> below.
>
>>   port 0: ovs-system (internal)
>>   port 1: genev_sys_6081 (geneve: packet_type=ptap)
>>   port 2: br-int (internal)
>>   port 3: br-ex (internal)
>>   port 4: eth2
>>   port 5: sw1p1 (internal)
>>   port 6: sw0p4 (internal)
>>
>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>> v2: - Changed precision to 2 digits
>>     - Handle missing kernel feature at netlink level
>> v3: - Rebase on the latest master branch
>>
>>  datapath/linux/compat/include/linux/openvswitch.h |    2 +-
>>  lib/dpctl.c                                       |    9 +++++++++
>>  lib/dpif-netdev.c                                 |    1 +
>>  lib/dpif-netlink.c                                |    9 +++++++++
>>  lib/dpif.h                                        |    2 ++
>>  5 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
>> b/datapath/linux/compat/include/linux/openvswitch.h
>> index 875de2025..690d78871 100644
>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>> @@ -122,8 +122,8 @@ struct ovs_dp_megaflow_stats {
>>  	__u64 n_mask_hit;	 /* Number of masks used for flow lookups. */
>>  	__u32 n_masks;		 /* Number of masks for the datapath. */
>>  	__u32 pad0;		 /* Pad for future expension. */
>> +	__u64 n_cache_hit;       /* Number of cache matches for flow 
>> lookups. */
>
> another nit: the datapath uses tab + spaces.
>
> Otherwise:
> Acked-by: Flavio Leitner <fbl at sysclose.org>

Thanks Flavio! Ilya do you need a v4 for the nits?

>>  	__u64 pad1;		 /* Pad for future expension. */
>> -	__u64 pad2;		 /* Pad for future expension. */
>>  };
>>
>>  struct ovs_vport_stats {
>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>> index ef8ae7402..acc677974 100644
>> --- a/lib/dpctl.c
>> +++ b/lib/dpctl.c
>> @@ -611,6 +611,15 @@ show_dpif(struct dpif *dpif, struct dpctl_params 
>> *dpctl_p)
>>              dpctl_print(dpctl_p, "  masks: hit:%"PRIu64" 
>> total:%"PRIu32
>>                                   " hit/pkt:%.2f\n",
>>                          stats.n_mask_hit, stats.n_masks, avg);
>> +
>> +            if (stats.n_cache_hit != UINT64_MAX) {
>> +                double avg_hits = n_pkts ?
>> +                    (double) stats.n_cache_hit / n_pkts * 100 : 0.0;
>> +
>> +                dpctl_print(dpctl_p,
>> +                            "  cache: hit:%"PRIu64" hit 
>> rate:%.2f%%\n",
>> +                            stats.n_cache_hit, avg_hits);
>> +            }
>>          }
>>      }
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 816945375..c3d3aac36 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -2025,6 +2025,7 @@ dpif_netdev_get_stats(const struct dpif *dpif, 
>> struct dpif_dp_stats *stats)
>>      }
>>      stats->n_masks = UINT32_MAX;
>>      stats->n_mask_hit = UINT64_MAX;
>> +    stats->n_cache_hit = UINT64_MAX;
>>
>>      return 0;
>>  }
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index ceb56c685..392b894c5 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -672,9 +672,18 @@ dpif_netlink_get_stats(const struct dpif *dpif_, 
>> struct dpif_dp_stats *stats)
>>              stats->n_masks = dp.megaflow_stats->n_masks;
>>              stats->n_mask_hit = get_32aligned_u64(
>>                  &dp.megaflow_stats->n_mask_hit);
>> +            stats->n_cache_hit = get_32aligned_u64(
>> +                &dp.megaflow_stats->n_cache_hit);
>> +
>> +            if (!stats->n_cache_hit) {
>> +                /* Old kernels don't use this field and always
>> +                 * report zero instead. Disable this stat. */
>> +                stats->n_cache_hit = UINT64_MAX;
>> +            }
>>          } else {
>>              stats->n_masks = UINT32_MAX;
>>              stats->n_mask_hit = UINT64_MAX;
>> +            stats->n_cache_hit = UINT64_MAX;
>>          }
>>          ofpbuf_delete(buf);
>>      }
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index ecda896c7..600e4522c 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -429,6 +429,8 @@ struct dpif_dp_stats {
>>      uint64_t n_missed;          /* Number of flow table misses. */
>>      uint64_t n_lost;            /* Number of misses not sent to 
>> userspace. */
>>      uint64_t n_flows;           /* Number of flows present. */
>> +    uint64_t n_cache_hit;       /* Number of mega flow mask cache 
>> hits for
>> +                                   flow table matches. */
>>      uint64_t n_mask_hit;        /* Number of mega flow masks visited 
>> for
>>                                     flow table matches. */
>>      uint32_t n_masks;           /* Number of mega flow masks. */
>>
>
> -- 
> fbl