[ovs-dev] upcall: Log failure to flow_put for dpif-netlink.

Previously these errors were only logged for dpif-netdev. Make it
consistent by merging the code for both datapaths.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
This could be handy for debugging on v2.6, so modulo any objections I
intend to apply it there too.
---
 ofproto/ofproto-dpif-upcall.c | 57 +++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

Looks goos except for a small style nit below.

Acked-by: Jarno Rajahalme <jarno@ovn.org>


> On Aug 18, 2016, at 2:50 PM, Joe Stringer <joe@ovn.org> wrote:
> 
> Previously these errors were only logged for dpif-netdev. Make it
> consistent by merging the code for both datapaths.
> 
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> This could be handy for debugging on v2.6, so modulo any objections I
> intend to apply it there too.
> ---
> ofproto/ofproto-dpif-upcall.c | 57 +++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index c83df9ea8648..9e1730b2c6d5 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1133,6 +1133,32 @@ upcall_uninit(struct upcall *upcall)
>     }
> }
> 
> +/* If there are less flows than the limit, and this is a miss upcall which
> + *
> + *      - Has no recirc_id, OR
> + *      - Has a recirc_id and we can get a reference on the recirc ctx,
> + *
> + * Then we should install the flow (true). Otherwise, return false. */
> +static bool should_install_flow(struct udpif *udpif, struct upcall *upcall)

“static bool” should be on the previous line.

> +{
> +    unsigned int flow_limit;
> +
> +    if (upcall->type != DPIF_UC_MISS) {
> +        return false;
> +    } else if (upcall->recirc && !upcall->have_recirc_ref) {
> +        VLOG_WARN_RL(&rl, "upcall: no reference for recirc flow");
> +        return false;
> +    }
> +
> +    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
> +    if (udpif_get_n_flows(udpif) >= flow_limit) {
> +        VLOG_WARN_RL(&rl, "upcall: datapath flow limit reached");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> static int
> upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufid,
>           unsigned pmd_id, enum dpif_upcall_type type,
> @@ -1141,13 +1167,11 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi
> {
>     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>     struct udpif *udpif = aux;
> -    unsigned int flow_limit;
>     struct upcall upcall;
>     bool megaflow;
>     int error;
> 
>     atomic_read_relaxed(&enable_megaflows, &megaflow);
> -    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
> 
>     error = upcall_receive(&upcall, udpif->backer, packet, type, userdata,
>                            flow, 0, ufid, pmd_id);
> @@ -1169,16 +1193,7 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi
>         flow_wildcards_init_for_packet(wc, flow);
>     }
> 
> -    if (udpif_get_n_flows(udpif) >= flow_limit) {
> -        VLOG_WARN_RL(&rl, "upcall_cb failure: datapath flow limit reached");
> -        error = ENOSPC;
> -        goto out;
> -    }
> -
> -    /* Prevent miss flow installation if the key has recirculation ID but we
> -     * were not able to get a reference on it. */
> -    if (type == DPIF_UC_MISS && upcall.recirc && !upcall.have_recirc_ref) {
> -        VLOG_WARN_RL(&rl, "upcall_cb failure: no reference for recirc flow");
> +    if (!should_install_flow(udpif, &upcall)) {
>         error = ENOSPC;
>         goto out;
>     }
> @@ -1297,13 +1312,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
> {
>     struct dpif_op *opsp[UPCALL_MAX_BATCH * 2];
>     struct ukey_op ops[UPCALL_MAX_BATCH * 2];
> -    unsigned int flow_limit;
>     size_t n_ops, n_opsp, i;
> -    bool may_put;
> -
> -    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
> -
> -    may_put = udpif_get_n_flows(udpif) < flow_limit;
> 
>     /* Handle the packets individually in order of arrival.
>      *
> @@ -1321,17 +1330,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>         const struct dp_packet *packet = upcall->packet;
>         struct ukey_op *op;
> 
> -        /* Do not install a flow into the datapath if:
> -         *
> -         *    - The datapath already has too many flows.
> -         *
> -         *    - We received this packet via some flow installed in the kernel
> -         *      already.
> -         *
> -         *    - Upcall was a recirculation but we do not have a reference to
> -         *      to the recirculation ID. */
> -        if (may_put && upcall->type == DPIF_UC_MISS &&
> -            (!upcall->recirc || upcall->have_recirc_ref)) {
> +        if (should_install_flow(udpif, upcall)) {
>             struct udpif_key *ukey = upcall->ukey;
> 
>             upcall->ukey_persists = true;
> -- 
> 2.9.2
>

On 18 August 2016 at 16:04, Jarno Rajahalme <jarno@ovn.org> wrote:
> Looks goos except for a small style nit below.
>
> Acked-by: Jarno Rajahalme <jarno@ovn.org>

Thanks, I made this change and applied the patch to master, branch-2.6.

>> On Aug 18, 2016, at 2:50 PM, Joe Stringer <joe@ovn.org> wrote:
>>
>> Previously these errors were only logged for dpif-netdev. Make it
>> consistent by merging the code for both datapaths.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>> ---
>> This could be handy for debugging on v2.6, so modulo any objections I
>> intend to apply it there too.
>> ---
>> ofproto/ofproto-dpif-upcall.c | 57 +++++++++++++++++++++----------------------
>> 1 file changed, 28 insertions(+), 29 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index c83df9ea8648..9e1730b2c6d5 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1133,6 +1133,32 @@ upcall_uninit(struct upcall *upcall)
>>     }
>> }
>>
>> +/* If there are less flows than the limit, and this is a miss upcall which
>> + *
>> + *      - Has no recirc_id, OR
>> + *      - Has a recirc_id and we can get a reference on the recirc ctx,
>> + *
>> + * Then we should install the flow (true). Otherwise, return false. */
>> +static bool should_install_flow(struct udpif *udpif, struct upcall *upcall)
>
> “static bool” should be on the previous line.
>
>> +{
>> +    unsigned int flow_limit;
>> +
>> +    if (upcall->type != DPIF_UC_MISS) {
>> +        return false;
>> +    } else if (upcall->recirc && !upcall->have_recirc_ref) {
>> +        VLOG_WARN_RL(&rl, "upcall: no reference for recirc flow");
>> +        return false;
>> +    }
>> +
>> +    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
>> +    if (udpif_get_n_flows(udpif) >= flow_limit) {
>> +        VLOG_WARN_RL(&rl, "upcall: datapath flow limit reached");
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> static int
>> upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufid,
>>           unsigned pmd_id, enum dpif_upcall_type type,
>> @@ -1141,13 +1167,11 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi
>> {
>>     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>     struct udpif *udpif = aux;
>> -    unsigned int flow_limit;
>>     struct upcall upcall;
>>     bool megaflow;
>>     int error;
>>
>>     atomic_read_relaxed(&enable_megaflows, &megaflow);
>> -    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
>>
>>     error = upcall_receive(&upcall, udpif->backer, packet, type, userdata,
>>                            flow, 0, ufid, pmd_id);
>> @@ -1169,16 +1193,7 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi
>>         flow_wildcards_init_for_packet(wc, flow);
>>     }
>>
>> -    if (udpif_get_n_flows(udpif) >= flow_limit) {
>> -        VLOG_WARN_RL(&rl, "upcall_cb failure: datapath flow limit reached");
>> -        error = ENOSPC;
>> -        goto out;
>> -    }
>> -
>> -    /* Prevent miss flow installation if the key has recirculation ID but we
>> -     * were not able to get a reference on it. */
>> -    if (type == DPIF_UC_MISS && upcall.recirc && !upcall.have_recirc_ref) {
>> -        VLOG_WARN_RL(&rl, "upcall_cb failure: no reference for recirc flow");
>> +    if (!should_install_flow(udpif, &upcall)) {
>>         error = ENOSPC;
>>         goto out;
>>     }
>> @@ -1297,13 +1312,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>> {
>>     struct dpif_op *opsp[UPCALL_MAX_BATCH * 2];
>>     struct ukey_op ops[UPCALL_MAX_BATCH * 2];
>> -    unsigned int flow_limit;
>>     size_t n_ops, n_opsp, i;
>> -    bool may_put;
>> -
>> -    atomic_read_relaxed(&udpif->flow_limit, &flow_limit);
>> -
>> -    may_put = udpif_get_n_flows(udpif) < flow_limit;
>>
>>     /* Handle the packets individually in order of arrival.
>>      *
>> @@ -1321,17 +1330,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>>         const struct dp_packet *packet = upcall->packet;
>>         struct ukey_op *op;
>>
>> -        /* Do not install a flow into the datapath if:
>> -         *
>> -         *    - The datapath already has too many flows.
>> -         *
>> -         *    - We received this packet via some flow installed in the kernel
>> -         *      already.
>> -         *
>> -         *    - Upcall was a recirculation but we do not have a reference to
>> -         *      to the recirculation ID. */
>> -        if (may_put && upcall->type == DPIF_UC_MISS &&
>> -            (!upcall->recirc || upcall->have_recirc_ref)) {
>> +        if (should_install_flow(udpif, upcall)) {
>>             struct udpif_key *ukey = upcall->ukey;
>>
>>             upcall->ukey_persists = true;
>> --
>> 2.9.2
>>
>