From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [RFC net-next 3/6] net: xdp: make the stack take care of the tear
 down
Date: Sat, 25 Nov 2017 00:24:50 +0100
Message-ID: <6b7dd6a9-3658-d1af-1d42-e898ccc10e5a@iogearbox.net>
References: <20171124023613.16855-1-jakub.kicinski@netronome.com>
 <20171124023613.16855-4-jakub.kicinski@netronome.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: oss-drivers@netronome.com, alexei.starovoitov@gmail.com,
        jiri@resnulli.us, Saeed Mahameed <saeedm@mellanox.com>,
        Michael Chan <michael.chan@broadcom.com>,
        Ariel Elior <Ariel.Elior@cavium.com>,
        John Fastabend <john.fastabend@gmail.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>,
        netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from www62.your-server.de ([213.133.104.62]:35289 "EHLO
        www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753730AbdKXXYy (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 24 Nov 2017 18:24:54 -0500
In-Reply-To: <20171124023613.16855-4-jakub.kicinski@netronome.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 11/24/2017 03:36 AM, Jakub Kicinski wrote:
> Since day one of XDP drivers had to remember to free the program
> on the remove path.  This leads to code duplication and is error
> prone.  Make the stack query the installed programs on unregister
> and if something is installed, remove the program.
> 
> Because the remove will now be called before notifiers are
> invoked, BPF offload state of the program will not get destroyed
> before uninstall.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
[...]

Nice work, series looks good to me! One really just minor
comment below:

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3f271c9cb5e0..a3e932f98419 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7110,6 +7110,23 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
>  	return bpf_op(dev, &xdp);
>  }
>  
> +static void dev_xdp_uninstall(struct net_device *dev)
> +{
> +	struct netdev_bpf xdp;
> +	bpf_op_t ndo_bpf;

Can you add a comment here stating that generic XDP does not
need to be handled since we drop the prog from free_netdev()?
Potentially we could also drop the generic one from here, that
way we'd make no difference and have a dev_xdp_install() and
one dev_xdp_uninstall() for all kind of attach types. Given
generic XDP should simulate native XDP anyway, probably better
to just do that.

> +	ndo_bpf = dev->netdev_ops->ndo_bpf;
> +	if (!ndo_bpf)
> +		return;
> +
> +	__dev_xdp_query(dev, ndo_bpf, &xdp);
> +	if (xdp.prog_attached == XDP_ATTACHED_NONE)
> +		return;
> +
> +	/* Program removal should always succeed */
> +	WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags, NULL));
> +}
> +
>  /**
>   *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
>   *	@dev: device
> @@ -7240,6 +7257,7 @@ static void rollback_registered_many(struct list_head *head)
>  		/* Shutdown queueing discipline. */
>  		dev_shutdown(dev);
>  
> +		dev_xdp_uninstall(dev);
>  
>  		/* Notify protocols, that we are about to destroy
>  		 * this device. They should clean all the things.
> 

Thanks,
Daniel