| Message ID | 163091841785.96839.4880012333775292574.stgit@ebuild |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dpctl: cache visibility/configuration enhancements | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi, Eelco. Sorry for this taking so long. The patch looks good to me, just one small nit below. On 9/6/21 10:53, Eelco Chaudron wrote: > This patch adds cache usage statistics to the output: > > $ ovs-dpctl show > system@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.21% The space in "hit rate" doesn't look right. All the other things are written as a single word followed by a semicolon and a number, so maybe this should be a single word too. E.g.: cache: hit:4 hit-rate:4.21% What do you think? I can make this change before applying a patch. Best regards, Ilya Maximets. > 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)
On 4 Nov 2021, at 15:51, Ilya Maximets wrote: > Hi, Eelco. Sorry for this taking so long. > > The patch looks good to me, just one small nit below. > > On 9/6/21 10:53, Eelco Chaudron wrote: >> This patch adds cache usage statistics to the output: >> >> $ ovs-dpctl show >> system@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.21% > > The space in "hit rate" doesn't look right. All the other things > are written as a single word followed by a semicolon and a number, > so maybe this should be a single word too. E.g.: > > cache: hit:4 hit-rate:4.21% > > What do you think? I can make this change before applying a patch. The change looks fine! Please make the change before applying. Thanks, Eelco > Best regards, Ilya Maximets. > >> 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)
On 11/5/21 10:20, Eelco Chaudron wrote: > > > On 4 Nov 2021, at 15:51, Ilya Maximets wrote: > >> Hi, Eelco. Sorry for this taking so long. >> >> The patch looks good to me, just one small nit below. >> >> On 9/6/21 10:53, Eelco Chaudron wrote: >>> This patch adds cache usage statistics to the output: >>> >>> $ ovs-dpctl show >>> system@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.21% >> >> The space in "hit rate" doesn't look right. All the other things >> are written as a single word followed by a semicolon and a number, >> so maybe this should be a single word too. E.g.: >> >> cache: hit:4 hit-rate:4.21% >> >> What do you think? I can make this change before applying a patch. > > The change looks fine! Please make the change before applying. > > Thanks, > > Eelco > >> Best regards, Ilya Maximets. >> >>> 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) > Thanks! Applied. Best regards, Ilya Maximets.
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index f0595eeba..a3f5fb919 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -126,8 +126,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 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 b3e57bb95..0d0623013 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1963,6 +1963,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 34fc04237..70751e634 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -749,9 +749,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 7c322d20e..b32ae5fc7 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. */