Git SHA: ada031dbdeae3eb0470e6ccea2b6eb0a1e318b87
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload: Add operate implementation to dpif-offload.
The patch adds APIs to support the dpif operate function to propagate to
the dpif-offload layer for flow manipulation and querying.
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index 1519ab5ed..64bb3242d 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
[ ... ]
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 62eef398b..da61a7c0e 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -31,7 +31,7 @@
>
> VLOG_DEFINE_THIS_MODULE(dpif_offload);
>
> -static struct vlog_rate_limit rl_dbg = VLOG_RATE_LIMIT_INIT(1, 5);
> +static struct vlog_rate_limit rl_dbg = VLOG_RATE_LIMIT_INIT(100, 100);
Was this rate limit change intentional in this commit? The commit message
doesn't mention adjusting logging rates, and this seems like it might
belong in a separate change or should be explained.
> @@ -1116,6 +1116,88 @@ dpif_offload_get_netdev_by_port_id(struct dpif *dpif,
> return netdev;
> }
>
> +/* This function tries to offload the operations to the dpif-offload
> + * providers. It will return the number of operations not handled, whose
> + * pointers are re-arranged and available in **ops. */
> +size_t
> +dpif_offload_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops,
> + enum dpif_offload_type offload_type)
> +{
[ ... ]
> + for (size_t i = 0; i < n_ops; i++) {
> + struct dpif_op *op = ops[i];
> +
> + if (offload_type == DPIF_OFFLOAD_ALWAYS) {
> + /* For DPIF_OFFLOAD_ALWAYS, we should keep the error values,
> + * and mark the unprocessed ones as EOPNOTSUPP. This way, they
> + * will not be processed by the dpif layer. */
The comment alignment looks incorrect - it uses spaces instead of proper
alignment. Also, notice the if statement block uses inconsistent
indentation (2 spaces instead of 4).
> + if (op->error < 0) {
> + op->error = EOPNOTSUPP;
> + }
> + continue;
> + }
> +
> + /* For the other offload types, operations that were not handled or
> + * failed to offload should be processed by the dpif layer. */
> + if (op->error != 0 && op->error != EEXIST) {
> + op->error = 0;
> + ops[n_ops_left++] = op;
> + }
> + }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 9efc38082..e8bbd65b7 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1401,7 +1401,25 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
> size_t n_ops,
> * handle itself, without help. */
> size_t i;
>
> - dpif->dpif_class->operate(dpif, ops, chunk, offload_type);
> + /* See if we can handle some flow add/del/get through the
> + * dpif-offload provider. */
> + if (offload_type != DPIF_OFFLOAD_NEVER
> + && dpif_offload_is_offload_enabled()) {
> + struct dpif_op **ops_copy;
> + size_t ops_left;
> +
> + ops_copy = xmemdup(ops, sizeof(*ops_copy) * chunk);
> +
> + ops_left = dpif_offload_operate(dpif, ops_copy, chunk,
> + offload_type);
> + if (ops_left) {
> + dpif->dpif_class->operate(dpif, ops_copy, ops_left,
> + offload_type);
> + }
> + free(ops_copy);
Does this code leak the ops_copy array when dpif_offload_operate fails?
The function doesn't appear to handle error returns from
dpif_offload_operate itself, only the individual operation errors. What
happens if dpif_offload_operate encounters an internal error and can't
process the operations?
Also, could this create issues with original operation results? The code
operates on ops_copy but the results from the copied operations aren't
being copied back to the original ops array.
> + } else {
> + dpif->dpif_class->operate(dpif, ops, chunk, offload_type);
> + }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
