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

Flavio Leitner fbl at sysclose.org
Mon Mar 1 21:04:41 UTC 2021


Hi Eelco,


Thanks for the patch. I added few comments in line below.


On Wed, Sep 02, 2020 at 12:12:36PM +0200, Eelco Chaudron wrote:
> This patch adds cache usage statistics to the output:
> 
> $ ovs-dpctl show                                                                                                                                                                                                                                                ovnhostvm: Tue Jul 21 15:25:14 2020


There are some extra spaces above

> 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%
>   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>
> ---
>  datapath/linux/compat/include/linux/openvswitch.h |    2 +-
>  lib/dpctl.c                                       |    9 +++++++++
>  lib/dpif-netdev.c                                 |    1 +
>  lib/dpif-netlink.c                                |    3 +++
>  lib/dpif.h                                        |    2 ++
>  5 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index cc41bbea4..fb73bfa90 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. */
>  	__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 db2b1f896..dac350fb5 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -605,6 +605,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 > 0 && stats.n_cache_hit != UINT64_MAX) {

The kernel uses memset() to zero all the struct stats, so unfortunately
we don't know if zero means unsupported/unused or just no hit yet.

One thing that troubles me is that when we start OVS and the stats
is zero, then there is no line and that might be unexpected to scripts
or users. Other stats show zero stats.

I am on the fence here because showing zero as well might confuse
users using old kernels without support, though it's a matter of time.

In any case, stats zero should be valid from dpctl point of view, so
this situation should be handled in the specific implementation which
in this case is netlink. Does that make sense? See below.


> +                double avg_hits = n_pkts ?
> +                    (double) stats.n_cache_hit / n_pkts * 100 : 0.0;
> +
> +                dpctl_print(dpctl_p,
> +                            "  cache: hit:%"PRIu64" hit rate:%.4f%%\n",

That's an interesting precision.
I'd say 2 decimal digits is enough. ;)

> +                            stats.n_cache_hit, avg_hits);
> +            }
>          }
>      }
>  
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 2aad24511..4e3bd7519 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2023,6 +2023,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 7da4fb54d..07f15ac65 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -672,9 +672,12 @@ 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);

I think we should work around the kernel issue here by checking
if it is zero and switch that to UINT64_MAX to disable it:

               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 2d52f0186..43c1ab998 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. */
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

-- 
fbl


More information about the dev mailing list