On 6/4/25 1:29 PM, Daniel Borkmann wrote:
> While testing, it turned out that upon error in the queue creation loop,
> we never trigger the af_xdp_cleanup() handler. This is because we pass
> errp instead of a local err pointer into the various AF_XDP setup functions
> instead of a scheme like:
>
> bool fn(..., Error **errp)
> {
> Error *err = NULL;
>
> foo(arg, &err);
> if (err) {
> handle the error...
> error_propagate(errp, err);
> return false;
> }
> ...
> }
>
> With a conversion into the above format, the af_xdp_cleanup() handler is
> called as expected.
How exactly this prevents calling the cleanup function? I don't see the
errp being checked anywhere in the qemu_del_net_client() path.
Could you provide a more detailed call sequence description where the cleanup
is not called?
I agree thought that the local err variable is actually unused. We should
be able to just remove it and remove the error_propagate() call as well.
> Also, making sure the XDP program will be removed does
> require to set s->n_queues to i + 1 since the test is nc->queue_index ==
> s->n_queues - 1, where nc->queue_index was set to i earlier.
The idea behind 'i' instead of 'i + 1' was that if either af_xdp_umem_create()
or af_xdp_socket_create() fails, we do not have xdp_flags initialized on the
last queue. And without it we can't remove the program, so we remove it while
destroying the last actually configured queue. And this is OK, because the
failed queue was not added to the program, and if the af_xdp_socket_create()
fails for the very first queue, then we don't have a program loaded at all.
With the new changes in this patch set, we have an extra function that can fail,
which is a new af_xdp_update_xsk_map(), and only if this one fails, we need to
remove the program while cleaning up the current failed queue, since it was
already created and xdp_flags are available.
If we get this patch as-is and the af_xdp_socket_create() fails, we will not
remove the program, AFAICT.
Or am I missing something?
Best regards, Ilya Maximets.
> With both
> fixed the cleanup triggers as expected. Note the error_propagate() handles
> a NULL err internally.
>
> Signed-off-by: Daniel Borkmann <dan...@iogearbox.net>
> Cc: Ilya Maximets <i.maxim...@ovn.org>
> Cc: Jason Wang <jasow...@redhat.com>
> Cc: Anton Protopopov <as...@isovalent.com>
> ---
> net/af-xdp.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/af-xdp.c b/net/af-xdp.c
> index b83d9bc47f..5d9857fdd8 100644
> --- a/net/af-xdp.c
> +++ b/net/af-xdp.c
> @@ -559,12 +559,11 @@ int net_init_af_xdp(const Netdev *netdev,
> s->map_start_index = opts->map_start_index;
> }
>
> - if (af_xdp_umem_create(s, sock_fds ? sock_fds[i] : -1, errp) ||
> - af_xdp_socket_create(s, opts, errp) ||
> - af_xdp_update_xsk_map(s, errp)) {
> + if (af_xdp_umem_create(s, sock_fds ? sock_fds[i] : -1, &err) ||
> + af_xdp_socket_create(s, opts, &err) ||
> + af_xdp_update_xsk_map(s, &err)) {
> /* Make sure the XDP program will be removed. */
> - s->n_queues = i;
> - error_propagate(errp, err);
> + s->n_queues = i + 1;
> goto err;
> }
> }
> @@ -586,6 +585,7 @@ int net_init_af_xdp(const Netdev *netdev,
> err:
> if (nc0) {
> qemu_del_net_client(nc0);
> + error_propagate(errp, err);
> }
>
> return -1;
