[ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch.

[Stokes, Ian]

> From: Harry van Haaren <harry.van.haaren at intel.com>
> 
> Optimize for the best case here where all packets will be compatible
> with 'netdev_flags'.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> Co-authored-by: Cian Ferriter <cian.ferriter at intel.com>
> Signed-off-by: Cian Ferriter <cian.ferriter at intel.com>

Thanks for the patch Cian/Harry, comments inline.

In general I have a concern of the impact on HWOL cases here and how much it would affect that. Have you data on this? Any thoughts from other HWOL folks here?
> ---
>  NEWS         |  2 ++
>  lib/netdev.c | 31 ++++++++++++++++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index a7ec34af1..8ad3c98db 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -18,6 +18,8 @@ Post-v2.15.0
>         CPU supports it. This enhances performance by using the native vpopcount
>         instructions, instead of the emulated version of vpopcount.
>       * Optimize dp_netdev_output by enhancing compiler optimization potential.
> +     * Optimize netdev sending by assuming the happy case, and using fallback
> +       for if the netdev doesnt meet the required HWOL needs of a packet.
This sounds a bit too low level for a NEWS item. Would suggest changing to single line Optimize netdev sending for best case scenario.

BR
Ian

>     - ovs-ctl:
>       * New option '--no-record-hostname' to disable hostname configuration
>         in ovsdb on startup.
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 91e91955c..29a5f1aa9 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -837,20 +837,33 @@ static void
>  netdev_send_prepare_batch(const struct netdev *netdev,
>                            struct dp_packet_batch *batch)
>  {
> -    struct dp_packet *packet;
> -    size_t i, size = dp_packet_batch_size(batch);
> +    struct dp_packet *p;
> +    uint32_t i, size = dp_packet_batch_size(batch);
> +    char *err_msg = NULL;
> 
> -    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
> -        char *errormsg = NULL;
> +    for (i = 0; i < size; i++) {
> +        p = batch->packets[i];
> +        int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg);
> 
> -        if (netdev_send_prepare_packet(netdev->ol_flags, packet, &errormsg)) {
> -            dp_packet_batch_refill(batch, packet, i);
> +        if (OVS_UNLIKELY(!pkt_ok)) {
> +            goto refill_loop;
> +        }
> +    }
> +
> +    return;
> +
> +refill_loop:
> +    /* Loop through packets from the start of the batch again. This is the
> +     * exceptional case where packets aren't compatible with 'netdev_flags'. */
> +    DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) {
> +        if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) {
> +            dp_packet_batch_refill(batch, p, i);
>          } else {
> -            dp_packet_delete(packet);
> +            dp_packet_delete(p);
>              COVERAGE_INC(netdev_send_prepare_drops);
>              VLOG_WARN_RL(&rl, "%s: Packet dropped: %s",
> -                         netdev_get_name(netdev), errormsg);
> -            free(errormsg);
> +                         netdev_get_name(netdev), err_msg);
> +            free(err_msg);
>          }
>      }
>  }
> --
> 2.31.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev