[ovs-dev] [PATCH V9 14/31] dpif-netlink: Use netdev flow put api to insert a flow

[Roi Dayan]

On 02/06/2017 23:08, Flavio Leitner wrote:
> On Sun, May 28, 2017 at 02:59:56PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb at mellanox.com>
>>
>> Using the new netdev flow api operate will now try and
>> offload flows to the relevant netdev of the input port.
>> Other operate methods flows will come in later patches.
>>
> [...]
>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 6f36b5e..8438e71 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
> [...]
>
>>  static void
>> -dpif_netlink_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops)
>> +dbg_print_flow(const struct nlattr *key, size_t key_len,
>> +               const struct nlattr *mask, size_t mask_len,
>> +               const struct nlattr *actions, size_t actions_len,
>> +               const ovs_u128 *ufid,
>> +               const char *op)
>> +{
>
>
> perhaps:
>       if (VLOG_IS_DBG_ENABLED()) {
>
>> +        struct ds s;
>> +
>> +        ds_init(&s);
>> +        ds_put_cstr(&s, op);
>> +        ds_put_cstr(&s, " (");
>> +        odp_format_ufid(ufid, &s);
>> +        ds_put_cstr(&s, ")");
>> +        if (key_len) {
>> +            ds_put_cstr(&s, "\nflow (verbose): ");
>> +            odp_flow_format(key, key_len, mask, mask_len, NULL, &s, true);
>> +            ds_put_cstr(&s, "\nflow: ");
>> +            odp_flow_format(key, key_len, mask, mask_len, NULL, &s, false);
>> +        }
>> +        ds_put_cstr(&s, "\nactions: ");
>> +        format_odp_actions(&s, actions, actions_len);
>> +        VLOG_DBG("\n%s", ds_cstr(&s));
>> +        ds_destroy(&s);
>      }
>
> to avoid those operations on every flow put?
> or maybe on the caller to avoid the function call.
>

right. maybe not an important change? in a later commit we remove
this function and refactor log_flow_put_message() from dpif.c and
use that. "dpif-netlink: Use dpif logging functions"


>
>> +}
>> +
>> +static int
>> +try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>>  {
>> -    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>> +    int err = EOPNOTSUPP;
>>
>> +    switch (op->type) {
>> +    case DPIF_OP_FLOW_PUT: {
>> +        struct dpif_flow_put *put = &op->u.flow_put;
>> +
>> +        if (!put->ufid) {
>> +            break;
>> +        }
>> +        dbg_print_flow(put->key, put->key_len, put->mask, put->mask_len,
>> +                       put->actions, put->actions_len, put->ufid,
>> +                       (put->flags & DPIF_FP_MODIFY ? "PUT(MODIFY)" : "PUT"));
>> +        err = parse_flow_put(dpif, put);
>> +        break;
>> +    }
>> +    case DPIF_OP_FLOW_DEL:
>> +    case DPIF_OP_FLOW_GET:
>> +    case DPIF_OP_EXECUTE:
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return err;
>> +}
>> +
>> +static void
>> +dpif_netlink_operate_chunks(struct dpif_netlink *dpif, struct dpif_op **ops,
>> +                            size_t n_ops)
>> +{
>>      while (n_ops > 0) {
>>          size_t chunk = dpif_netlink_operate__(dpif, ops, n_ops);
>> +
>>          ops += chunk;
>>          n_ops -= chunk;
>>      }
>>  }
>>
>> +static void
>> +dpif_netlink_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops)
>> +{
>> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>> +    struct dpif_op *new_ops[OPERATE_MAX_OPS];
>> +    int count = 0;
>> +    int i = 0;
>> +    int err = 0;
>> +
>> +    if (netdev_is_flow_api_enabled()) {
>> +        while (n_ops > 0) {
>> +            count = 0;
>> +
>> +            while (n_ops > 0 && count < OPERATE_MAX_OPS) {
>> +                struct dpif_op *op = ops[i++];
>> +
>> +                err = try_send_to_netdev(dpif, op);
>> +                if (err && err != EEXIST) {
>> +                    new_ops[count++] = op;
>> +                } else {
>> +                    op->error = err;
>> +                }
>> +
>> +                n_ops--;
>> +            }
>> +
>> +            dpif_netlink_operate_chunks(dpif, new_ops, count);
>> +        }
>> +
>> +        return;
>> +    }
>> +
>> +    dpif_netlink_operate_chunks(dpif, ops, n_ops);
>> +}
>> +
>
> The above could be:
>
>     if (netdev_is_flow_api_enabled()) {
>         ...
>     } else {
>        dpif_netlink_operate_chunks(dpif, ops, n_ops);
>     }
>
>    return;
> }
>
>
> Otherwise the patch looks good to me.
>

ok