Re: [patch net-next v2 4/7] switchdev: introduce possibility to defer obj_add/del
Mon, Oct 12, 2015 at 04:34:25PM CEST, niko...@cumulusnetworks.com wrote: >On 10/12/2015 03:15 PM, Jiri Pirko wrote: >> From: Jiri Pirko>> >> Similar to the attr usecase, the caller knows if he is holding RTNL and is >> in atomic section. So let the called to decide the correct call variant. >> >> This allows drivers to sleep inside their ops and wait for hw to get the >> operation status. Then the status is propagated into switchdev core. >> This avoids silent errors in drivers. >> >> Signed-off-by: Jiri Pirko >> --- >> include/net/switchdev.h | 1 + >> net/switchdev/switchdev.c | 137 >> +- >> 2 files changed, 112 insertions(+), 26 deletions(-) >> >[snip] >> + >> +struct switchdev_obj_work { >> +struct work_struct work; >> +struct net_device *dev; >> +struct switchdev_obj obj; >> +bool add; /* add of del */ >s/of/or/ ? :-) will fix, thanks. > >> +}; >> + >> +static void switchdev_port_obj_work(struct work_struct *work) >> +{ >> +struct switchdev_obj_work *ow = >> +container_of(work, struct switchdev_obj_work, work); >> +bool rtnl_locked = rtnl_is_locked(); >> +int err; >> + >> +if (!rtnl_locked) >> +rtnl_lock(); >> +if (ow->add) >> +err = switchdev_port_obj_add_now(ow->dev, >obj); >> +else >> +err = switchdev_port_obj_del_now(ow->dev, >obj); >> +if (err && err != -EOPNOTSUPP) >> +netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n", >> + err, ow->add ? "add" : "del", ow->obj.id); >> +if (!rtnl_locked) >> +rtnl_unlock(); >> + >> +dev_put(ow->dev); >> +kfree(ow); >> +} >> + >> +static int switchdev_port_obj_work_schedule(struct net_device *dev, >> +const struct switchdev_obj *obj, >> +bool add) >> +{ >> +struct switchdev_obj_work *ow; >> + >> +ow = kmalloc(sizeof(*ow), GFP_ATOMIC); >> +if (!ow) >> +return -ENOMEM; >> + >> +INIT_WORK(>work, switchdev_port_obj_work); >> + >This can be called without rtnl, what stops the device from disappearing >between the above and the hold below ? You are right. I will have to figure that out. Btw the same issue already exists for attr_set deferred work. > >> +dev_hold(dev); >> +ow->dev = dev; >> +memcpy(>obj, obj, sizeof(ow->obj)); >> +ow->add = add; >> + >> +queue_work(switchdev_wq, >work); >> +return 0; >> +} >> + >[snip] > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 4/7] switchdev: introduce possibility to defer obj_add/del
On 10/12/2015 04:34 PM, Nikolay Aleksandrov wrote: > On 10/12/2015 03:15 PM, Jiri Pirko wrote: >> From: Jiri Pirko>> >> Similar to the attr usecase, the caller knows if he is holding RTNL and is >> in atomic section. So let the called to decide the correct call variant. >> >> This allows drivers to sleep inside their ops and wait for hw to get the >> operation status. Then the status is propagated into switchdev core. >> This avoids silent errors in drivers. >> >> Signed-off-by: Jiri Pirko >> --- >> include/net/switchdev.h | 1 + >> net/switchdev/switchdev.c | 137 >> +- >> 2 files changed, 112 insertions(+), 26 deletions(-) >> > [snip] >> + >> +struct switchdev_obj_work { >> +struct work_struct work; >> +struct net_device *dev; >> +struct switchdev_obj obj; >> +bool add; /* add of del */ > s/of/or/ ? :-) > >> +}; >> + >> +static void switchdev_port_obj_work(struct work_struct *work) >> +{ >> +struct switchdev_obj_work *ow = >> +container_of(work, struct switchdev_obj_work, work); >> +bool rtnl_locked = rtnl_is_locked(); >> +int err; >> + >> +if (!rtnl_locked) >> +rtnl_lock(); >> +if (ow->add) >> +err = switchdev_port_obj_add_now(ow->dev, >obj); >> +else >> +err = switchdev_port_obj_del_now(ow->dev, >obj); >> +if (err && err != -EOPNOTSUPP) >> +netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n", >> + err, ow->add ? "add" : "del", ow->obj.id); >> +if (!rtnl_locked) >> +rtnl_unlock(); >> + >> +dev_put(ow->dev); >> +kfree(ow); >> +} >> + >> +static int switchdev_port_obj_work_schedule(struct net_device *dev, >> +const struct switchdev_obj *obj, >> +bool add) >> +{ >> +struct switchdev_obj_work *ow; >> + >> +ow = kmalloc(sizeof(*ow), GFP_ATOMIC); >> +if (!ow) >> +return -ENOMEM; >> + >> +INIT_WORK(>work, switchdev_port_obj_work); >> + > This can be called without rtnl, what stops the device from disappearing > between the above and the hold below ? > Nevermind this question, got it. Cheers, Nik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 4/7] switchdev: introduce possibility to defer obj_add/del
On 10/12/2015 04:44 PM, Jiri Pirko wrote: > Mon, Oct 12, 2015 at 04:34:25PM CEST, niko...@cumulusnetworks.com wrote: >> On 10/12/2015 03:15 PM, Jiri Pirko wrote: >>> From: Jiri Pirko>>> >>> Similar to the attr usecase, the caller knows if he is holding RTNL and is >>> in atomic section. So let the called to decide the correct call variant. >>> >>> This allows drivers to sleep inside their ops and wait for hw to get the >>> operation status. Then the status is propagated into switchdev core. >>> This avoids silent errors in drivers. >>> >>> Signed-off-by: Jiri Pirko >>> --- >>> include/net/switchdev.h | 1 + >>> net/switchdev/switchdev.c | 137 >>> +- >>> 2 files changed, 112 insertions(+), 26 deletions(-) >>> >> [snip] >>> + >>> +struct switchdev_obj_work { >>> + struct work_struct work; >>> + struct net_device *dev; >>> + struct switchdev_obj obj; >>> + bool add; /* add of del */ >> s/of/or/ ? :-) > > will fix, thanks. > > >> >>> +}; >>> + >>> +static void switchdev_port_obj_work(struct work_struct *work) >>> +{ >>> + struct switchdev_obj_work *ow = >>> + container_of(work, struct switchdev_obj_work, work); >>> + bool rtnl_locked = rtnl_is_locked(); >>> + int err; >>> + >>> + if (!rtnl_locked) >>> + rtnl_lock(); >>> + if (ow->add) >>> + err = switchdev_port_obj_add_now(ow->dev, >obj); >>> + else >>> + err = switchdev_port_obj_del_now(ow->dev, >obj); >>> + if (err && err != -EOPNOTSUPP) >>> + netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n", >>> + err, ow->add ? "add" : "del", ow->obj.id); >>> + if (!rtnl_locked) >>> + rtnl_unlock(); >>> + >>> + dev_put(ow->dev); >>> + kfree(ow); >>> +} >>> + >>> +static int switchdev_port_obj_work_schedule(struct net_device *dev, >>> + const struct switchdev_obj *obj, >>> + bool add) >>> +{ >>> + struct switchdev_obj_work *ow; >>> + >>> + ow = kmalloc(sizeof(*ow), GFP_ATOMIC); >>> + if (!ow) >>> + return -ENOMEM; >>> + >>> + INIT_WORK(>work, switchdev_port_obj_work); >>> + >> This can be called without rtnl, what stops the device from disappearing >> between the above and the hold below ? > > You are right. I will have to figure that out. Btw the same issue > already exists for attr_set deferred work. > > I have to say there're a few users now that need delayed RTNL execution the bonding being a heavy one, teaming I think also has some rtnl delays. Maybe it's time we do a generic delayed rtnl execution so it can be re-used by all. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 4/7] switchdev: introduce possibility to defer obj_add/del
On 10/12/2015 03:15 PM, Jiri Pirko wrote: > From: Jiri Pirko> > Similar to the attr usecase, the caller knows if he is holding RTNL and is > in atomic section. So let the called to decide the correct call variant. > > This allows drivers to sleep inside their ops and wait for hw to get the > operation status. Then the status is propagated into switchdev core. > This avoids silent errors in drivers. > > Signed-off-by: Jiri Pirko > --- > include/net/switchdev.h | 1 + > net/switchdev/switchdev.c | 137 > +- > 2 files changed, 112 insertions(+), 26 deletions(-) > [snip] > + > +struct switchdev_obj_work { > + struct work_struct work; > + struct net_device *dev; > + struct switchdev_obj obj; > + bool add; /* add of del */ s/of/or/ ? :-) > +}; > + > +static void switchdev_port_obj_work(struct work_struct *work) > +{ > + struct switchdev_obj_work *ow = > + container_of(work, struct switchdev_obj_work, work); > + bool rtnl_locked = rtnl_is_locked(); > + int err; > + > + if (!rtnl_locked) > + rtnl_lock(); > + if (ow->add) > + err = switchdev_port_obj_add_now(ow->dev, >obj); > + else > + err = switchdev_port_obj_del_now(ow->dev, >obj); > + if (err && err != -EOPNOTSUPP) > + netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n", > +err, ow->add ? "add" : "del", ow->obj.id); > + if (!rtnl_locked) > + rtnl_unlock(); > + > + dev_put(ow->dev); > + kfree(ow); > +} > + > +static int switchdev_port_obj_work_schedule(struct net_device *dev, > + const struct switchdev_obj *obj, > + bool add) > +{ > + struct switchdev_obj_work *ow; > + > + ow = kmalloc(sizeof(*ow), GFP_ATOMIC); > + if (!ow) > + return -ENOMEM; > + > + INIT_WORK(>work, switchdev_port_obj_work); > + This can be called without rtnl, what stops the device from disappearing between the above and the hold below ? > + dev_hold(dev); > + ow->dev = dev; > + memcpy(>obj, obj, sizeof(ow->obj)); > + ow->add = add; > + > + queue_work(switchdev_wq, >work); > + return 0; > +} > + [snip] -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 4/7] switchdev: introduce possibility to defer obj_add/del
Mon, Oct 12, 2015 at 04:42:12PM CEST, niko...@cumulusnetworks.com wrote: >On 10/12/2015 04:34 PM, Nikolay Aleksandrov wrote: >> On 10/12/2015 03:15 PM, Jiri Pirko wrote: >>> From: Jiri Pirko>>> >>> Similar to the attr usecase, the caller knows if he is holding RTNL and is >>> in atomic section. So let the called to decide the correct call variant. >>> >>> This allows drivers to sleep inside their ops and wait for hw to get the >>> operation status. Then the status is propagated into switchdev core. >>> This avoids silent errors in drivers. >>> >>> Signed-off-by: Jiri Pirko >>> --- >>> include/net/switchdev.h | 1 + >>> net/switchdev/switchdev.c | 137 >>> +- >>> 2 files changed, 112 insertions(+), 26 deletions(-) >>> >> [snip] >>> + >>> +struct switchdev_obj_work { >>> + struct work_struct work; >>> + struct net_device *dev; >>> + struct switchdev_obj obj; >>> + bool add; /* add of del */ >> s/of/or/ ? :-) >> >>> +}; >>> + >>> +static void switchdev_port_obj_work(struct work_struct *work) >>> +{ >>> + struct switchdev_obj_work *ow = >>> + container_of(work, struct switchdev_obj_work, work); >>> + bool rtnl_locked = rtnl_is_locked(); >>> + int err; >>> + >>> + if (!rtnl_locked) >>> + rtnl_lock(); >>> + if (ow->add) >>> + err = switchdev_port_obj_add_now(ow->dev, >obj); >>> + else >>> + err = switchdev_port_obj_del_now(ow->dev, >obj); >>> + if (err && err != -EOPNOTSUPP) >>> + netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n", >>> + err, ow->add ? "add" : "del", ow->obj.id); >>> + if (!rtnl_locked) >>> + rtnl_unlock(); >>> + >>> + dev_put(ow->dev); >>> + kfree(ow); >>> +} >>> + >>> +static int switchdev_port_obj_work_schedule(struct net_device *dev, >>> + const struct switchdev_obj *obj, >>> + bool add) >>> +{ >>> + struct switchdev_obj_work *ow; >>> + >>> + ow = kmalloc(sizeof(*ow), GFP_ATOMIC); >>> + if (!ow) >>> + return -ENOMEM; >>> + >>> + INIT_WORK(>work, switchdev_port_obj_work); >>> + >> This can be called without rtnl, what stops the device from disappearing >> between the above and the hold below ? >> >Nevermind this question, got it. Well anyone without rtnl should hold dev in the first place. So this should be ok. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html