Re: [PULL 07/20] virtio-net: Do not propagate ebpf-rss-fds errors

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jun 04, 2024 at 03:37:42PM +0800, Jason Wang wrote:
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Propagating ebpf-rss-fds errors has several problems.
> 
> First, it makes device realization fail and disables the fallback to the
> conventional eBPF loading.

AFAICT, this is not a bug - this is desired behaviour.

If the user/mgmt app has told QEMU to use FDs it has passed
in, then any failure to do this *MUST* be treated as a fatal
error. Falling back to other codepaths is ignoring a direct
user request.

> Second, it leaks memory by making device realization fail without
> freeing memory already allocated.
> 
> Third, the convention is to set an error when a function returns false,
> but virtio_net_load_ebpf_fds() and virtio_net_load_ebpf() returns false
> without setting an error, which is confusing.
> 
> Remove the propagation to fix these problems.

It doesn't fix the problems. It ignores the 2nd and 3rd problems
by removing the error reporting and ignoring the users' requested
config.

> 
> Fixes: 0524ea0510a3 ("ebpf: Added eBPF initialization by fds.")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index ff600b3002..3cee2ef3ac 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1329,24 +1329,22 @@ static void virtio_net_detach_epbf_rss(VirtIONet *n)
>      virtio_net_attach_ebpf_to_backend(n->nic, -1);
>  }
>  
> -static bool virtio_net_load_ebpf_fds(VirtIONet *n, Error **errp)
> +static bool virtio_net_load_ebpf_fds(VirtIONet *n)
>  {
>      int fds[EBPF_RSS_MAX_FDS] = { [0 ... EBPF_RSS_MAX_FDS - 1] = -1};
>      int ret = true;
>      int i = 0;
>  
> -    ERRP_GUARD();
> -
>      if (n->nr_ebpf_rss_fds != EBPF_RSS_MAX_FDS) {
> -        error_setg(errp,
> -                  "Expected %d file descriptors but got %d",
> -                  EBPF_RSS_MAX_FDS, n->nr_ebpf_rss_fds);
> +        warn_report("Expected %d file descriptors but got %d",
> +                    EBPF_RSS_MAX_FDS, n->nr_ebpf_rss_fds);
>         return false;
>     }
>  
>      for (i = 0; i < n->nr_ebpf_rss_fds; i++) {
> -        fds[i] = monitor_fd_param(monitor_cur(), n->ebpf_rss_fds[i], errp);
> -        if (*errp) {
> +        fds[i] = monitor_fd_param(monitor_cur(), n->ebpf_rss_fds[i],
> +                                  &error_warn);
> +        if (fds[i] < 0) {
>              ret = false;
>              goto exit;
>          }
> @@ -1355,7 +1353,7 @@ static bool virtio_net_load_ebpf_fds(VirtIONet *n, Error **errp)
>      ret = ebpf_rss_load_fds(&n->ebpf_rss, fds[0], fds[1], fds[2], fds[3]);
>  
>  exit:
> -    if (!ret || *errp) {
> +    if (!ret) {
>          for (i = 0; i < n->nr_ebpf_rss_fds && fds[i] != -1; i++) {
>              close(fds[i]);
>          }
> @@ -1364,13 +1362,12 @@ exit:
>      return ret;
>  }
>  
> -static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
> +static bool virtio_net_load_ebpf(VirtIONet *n)
>  {
>      bool ret = false;
>  
>      if (virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
> -        if (!(n->ebpf_rss_fds
> -                && virtio_net_load_ebpf_fds(n, errp))) {
> +        if (!(n->ebpf_rss_fds && virtio_net_load_ebpf_fds(n))) {
>              ret = ebpf_rss_load(&n->ebpf_rss);
>          }
>      }
> @@ -3809,7 +3806,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>      net_rx_pkt_init(&n->rx_pkt);
>  
>      if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
> -        virtio_net_load_ebpf(n, errp);
> +        virtio_net_load_ebpf(n);
>      }
>  }
>  
> -- 
> 2.42.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|