On Tue, Jun 04, 2024 at 03:37:42PM +0800, Jason Wang wrote:
> From: Akihiko Odaki <akihiko.od...@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.od...@daynix.com>
> Signed-off-by: Jason Wang <jasow...@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 :|
