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.kicin...@netronome.com>
> Reviewed-by: Simon Horman <simon.hor...@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
