[ovs-dev] [PATCH v4 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes

[Paolo Valerio]

Eelco Chaudron <echaudro at redhat.com> writes:

> On 1 Jun 2021, at 12:13, Paolo Valerio wrote:
>
>> Eelco Chaudron <echaudro at redhat.com> writes:
>>
>>> This patch adds a general way of viewing/configuring datapath
>>> cache sizes. With an implementation for the netlink interface.
>>>
>>> The ovs-dpctl/ovs-appctl show commands will display the
>>> current cache sizes configured:
>>>
>>> ovs-dpctl show
>>> system at ovs-system:
>>>   lookups: hit:25 missed:63 lost:0
>>>   flows: 0
>>>   masks: hit:282 total:0 hit/pkt:3.20
>>>   cache: hit:4 hit rate:4.54%
>>>   caches:
>>>     masks-cache: size: 256
>>>   port 0: ovs-system (internal)
>>>   port 1: br-int (internal)
>>>   port 2: genev_sys_6081 (geneve: packet_type=ptap)
>>>   port 3: br-ex (internal)
>>>   port 4: eth2
>>>   port 5: sw0p1 (internal)
>>>   port 6: sw0p3 (internal)
>>>
>>> A specific cache can be configured as follows:
>>>
>>> ovs-appctl dpctl/cache-set-size DP CACHE SIZE
>>> ovs-dpctl cache-set-size DP CACHE SIZE
>>>
>>> For example to disable the cache do:
>>>
>>> $ ovs-dpctl cache-set-size system at ovs-system masks-cache 0
>>> Setting cache size successful, new size 0.
>>>
>>> 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
>>> v4: - Fixed commit message
>>>     - Fix issue with resetting user_features
>>>
>>>  datapath/linux/compat/include/linux/openvswitch.h |    3 -
>>>  lib/dpctl.c                                       |  120 +++++++++++++++++++++
>>>  lib/dpctl.man                                     |   14 ++
>>>  lib/dpif-netdev.c                                 |    4 +
>>>  lib/dpif-netlink.c                                |  113 ++++++++++++++++++++
>>>  lib/dpif-provider.h                               |   20 ++++
>>>  lib/dpif.c                                        |   32 ++++++
>>>  lib/dpif.h                                        |    7 +
>>>  tests/system-traffic.at                           |   36 ++++++
>>>  utilities/ovs-dpctl.c                             |    4 +
>>>  10 files changed, 352 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
>>> index 690d78871..35f32d20c 100644
>>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>>> @@ -105,6 +105,7 @@ enum ovs_datapath_attr {
>>>  	OVS_DP_ATTR_MEGAFLOW_STATS,	/* struct ovs_dp_megaflow_stats */
>>>  	OVS_DP_ATTR_USER_FEATURES,	/* OVS_DP_F_*  */
>>>  	OVS_DP_ATTR_PAD,
>>> +	OVS_DP_ATTR_MASKS_CACHE_SIZE,
>>>  	__OVS_DP_ATTR_MAX
>>>  };
>>>
>>> @@ -122,7 +123,7 @@ 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 n_cache_hit;	 /* Number of cache matches for flow lookups. */
>>
>> the previous patch introduces this member, was it intentional to replace
>> spaces with tabs in this patch?
>>
>
> Yes, this was a comment from Flavio in the previous revision.
>

Sorry, I didn't make it clear.

What I meant was to replace the spaces in the first patch instead of
doing it in the second, but I just noticed you did it already in the
v5, so thanks.

>>>  	__u64 pad1;		 /* Pad for future expension. */
>>>  };
>>
>> [...]
>>
>>> +static int
>>> +dpif_netlink_cache_set_size(struct dpif *dpif_, uint32_t level, uint32_t size)
>>> +{
>>> +    int error;
>>> +    struct ofpbuf *bufp;
>>> +    struct dpif_netlink_dp request, reply;
>>> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>> +
>>> +    size = ROUND_UP_POW2(size);
>>> +
>>> +    if (level != 0) {
>>> +        return EINVAL;
>>> +    }
>>> +
>>> +    dpif_netlink_dp_init(&request);
>>> +    request.cmd = OVS_DP_CMD_SET;
>>> +    request.name = dpif_->base_name;
>>> +    request.dp_ifindex = dpif->dp_ifindex;
>>> +    request.cache_size = size;
>>> +
>>
>> I don't see any explicit reference to user_features.
>> Am I missing something?
>
> Nope, I forgot to do an stg refresh :( I will sent out a v5, thanks for noticing.
>
>
>>> +    error = dpif_netlink_dp_transact(&request, &reply, &bufp);
>>> +    if (!error) {
>>> +        ofpbuf_delete(bufp);
>>> +        if (reply.cache_size != size) {
>>> +            return EINVAL;
>>> +        }
>>> +    }
>>> +
>>> +    return error;
>>> +}
>>> +