Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink

[David Ahern]

On 3/23/18 9:47 PM, Jakub Kicinski wrote:
> On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
>> From: David Ahern <dsah...@gmail.com>
>>
>> Add devlink support to netdevsim and use it to implement a simple,
>> profile based resource controller. Only one controller is needed
>> per namespace, so the first netdevsim netdevice in a namespace
>> registers with devlink. If that device is deleted, the resource
>> settings are deleted.
> 
> FWIW some nits from me blow.
> 
>> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>> index 09388c06171d..449b2a1a1800 100644
>> --- a/drivers/net/netdevsim/Makefile
>> +++ b/drivers/net/netdevsim/Makefile
>> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
>>  netdevsim-objs += \
>>      bpf.o
>>  endif
>> +
>> +ifneq ($(CONFIG_NET_DEVLINK),)
> 
> Hm.  Don't you need MAY_USE_DEVLINK dependency perhaps?

mlxsw uses CONFIG_NET_DEVLINK in its Makefile.

MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me
why it is needed at all.

> 
>> +netdevsim-objs += devlink.o fib.o
>> +endif
>> diff --git a/drivers/net/netdevsim/devlink.c 
>> b/drivers/net/netdevsim/devlink.c
>> new file mode 100644
>> index 000000000000..d10558e1b022
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/devlink.c
>  
>> +static int devlink_resources_register(struct devlink *devlink)
>> +{
>> +    struct devlink_resource_size_params params = {
>> +            .size_max = (u64)-1,
>> +            .size_granularity = 1,
>> +            .unit = DEVLINK_RESOURCE_UNIT_ENTRY
>> +    };
>> +    struct net *net = devlink_net(devlink);
>> +    int err;
>> +    u64 n;
>> +
>> +    /* Resources for IPv4 */
>> +    err = devlink_resource_register(devlink, "IPv4", (u64)-1,
>> +                                    NSIM_RESOURCE_IPV4,
>> +                                    DEVLINK_RESOURCE_ID_PARENT_TOP,
>> +                                    &params, NULL);
>> +    if (err) {
>> +            pr_err("Failed to register IPv4 top resource\n");
>> +            goto out;
> 
> nit: why goto out here and return err everywhere else?  If I was to
>      choose I'd rather see returns.  goto X; X: return; is less
>      obviously correct IMHO.  Besides labels should be called by the
>      action they perform/undo, so goto err_return? :S

Will fix. Just got lost in the many iterations leading up to the RFC.

> 
>> +    }
>> +
>> +    n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
>> +    err = devlink_resource_register(devlink, "fib", n,
>> +                                    NSIM_RESOURCE_IPV4_FIB,
>> +                                    NSIM_RESOURCE_IPV4,
>> +                                    &params, &nsim_ipv4_fib_res_ops);
>> +    if (err) {
>> +            pr_err("Failed to register IPv4 FIB resource\n");
>> +            return err;
>> +    }
>> +
>> +    n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
>> +    err = devlink_resource_register(devlink, "fib-rules", n,
>> +                                    NSIM_RESOURCE_IPV4_FIB_RULES,
>> +                                    NSIM_RESOURCE_IPV4,
>> +                                    &params, &nsim_ipv4_fib_rules_res_ops);
>> +    if (err) {
>> +            pr_err("Failed to register IPv4 FIB rules resource\n");
>> +            return err;
>> +    }
>> +
>> +    /* Resources for IPv6 */
>> +    err = devlink_resource_register(devlink, "IPv6", (u64)-1,
>> +                                    NSIM_RESOURCE_IPV6,
>> +                                    DEVLINK_RESOURCE_ID_PARENT_TOP,
>> +                                    &params, NULL);
>> +    if (err) {
>> +            pr_err("Failed to register IPv6 top resource\n");
>> +            goto out;
>> +    }
>> +
>> +    n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
>> +    err = devlink_resource_register(devlink, "fib", n,
>> +                                    NSIM_RESOURCE_IPV6_FIB,
>> +                                    NSIM_RESOURCE_IPV6,
>> +                                    &params, &nsim_ipv6_fib_res_ops);
>> +    if (err) {
>> +            pr_err("Failed to register IPv6 FIB resource\n");
>> +            return err;
>> +    }
>> +
>> +    n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
>> +    err = devlink_resource_register(devlink, "fib-rules", n,
>> +                                    NSIM_RESOURCE_IPV6_FIB_RULES,
>> +                                    NSIM_RESOURCE_IPV6,
>> +                                    &params, &nsim_ipv6_fib_rules_res_ops);
>> +    if (err) {
>> +            pr_err("Failed to register IPv6 FIB rules resource\n");
>> +            return err;
>> +    }
>> +out:
>> +    return err;
>> +}
> 
>> +void nsim_devlink_teardown(struct netdevsim *ns)
>> +{
>> +    if (ns->devlink) {
>> +            struct net *net = dev_net(ns->netdev);
>> +            bool *reg_devlink = net_generic(net, nsim_devlink_id);
> 
> nit: reverse xmas tree

reg_devlink uses net, so the order can not be changed

> 
>> +            devlink_unregister(ns->devlink);
>> +            devlink_free(ns->devlink);
>> +            ns->devlink = NULL;
>> +
>> +            nsim_devlink_net_reset(net);
>> +            *reg_devlink = true;
>> +    }
>> +}
> 
>> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>> new file mode 100644
>> index 000000000000..b77dcafc7158
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/fib.c
> 
>> +static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
>> +                         void *ptr)
>> +{
>> +    struct fib_notifier_info *info = ptr;
>> +    int err;
>> +
>> +    switch (event) {
>> +    case FIB_EVENT_RULE_ADD: /* fall through */
> 
> nit: I don't think fall through comment is needed for back-to-back
>      cases.

ok.

Thanks for the review.

> 
>> +    case FIB_EVENT_RULE_DEL:
>> +            err = nsim_fib_rule_event(info, event == FIB_EVENT_RULE_ADD);
>> +            break;
>> +
>> +    case FIB_EVENT_ENTRY_ADD:  /* fall through */
>> +    case FIB_EVENT_ENTRY_DEL:
>> +            err = nsim_fib_event(info, event == FIB_EVENT_ENTRY_ADD);
>> +            break;
>> +    }
>> +
>> +    return notifier_from_errno(err);
>> +}