Re: [PATCH net-next v2 05/10] net: sched: use Qdisc rcu API instead of relying on rtnl lock
On Thu, Sep 20, 2018 at 12:21 AM Vlad Buslov wrote: > > > On Wed 19 Sep 2018 at 22:04, Cong Wang wrote: > > On Mon, Sep 17, 2018 at 12:19 AM Vlad Buslov wrote: > >> +static void tcf_qdisc_put(struct Qdisc *q, bool rtnl_held) > >> +{ > >> + if (!q) > >> + return; > >> + > >> + if (rtnl_held) > >> + qdisc_put(q); > >> + else > >> + qdisc_put_unlocked(q); > >> +} > > > > This is very ugly. You should know whether RTNL is held or > > not when calling it. > > > > What's more, all of your code passes true, so why do you > > need a parameter for rtnl_held? > > It passes true because currently rule update handlers still registered > as locked. This is a preparation for next patch set where this would be > changed to proper variable that depends on qdics and classifier type. You can always add it when you really need it. I doubt you need such a tiny wrapper even in the next patchset, as it can be easily folded into callers.
Re: [PATCH net-next v2 05/10] net: sched: use Qdisc rcu API instead of relying on rtnl lock
On Wed 19 Sep 2018 at 22:04, Cong Wang wrote: > On Mon, Sep 17, 2018 at 12:19 AM Vlad Buslov wrote: >> +static void tcf_qdisc_put(struct Qdisc *q, bool rtnl_held) >> +{ >> + if (!q) >> + return; >> + >> + if (rtnl_held) >> + qdisc_put(q); >> + else >> + qdisc_put_unlocked(q); >> +} > > This is very ugly. You should know whether RTNL is held or > not when calling it. > > What's more, all of your code passes true, so why do you > need a parameter for rtnl_held? It passes true because currently rule update handlers still registered as locked. This is a preparation for next patch set where this would be changed to proper variable that depends on qdics and classifier type.
Re: [PATCH net-next v2 05/10] net: sched: use Qdisc rcu API instead of relying on rtnl lock
On Mon, Sep 17, 2018 at 12:19 AM Vlad Buslov wrote: > +static void tcf_qdisc_put(struct Qdisc *q, bool rtnl_held) > +{ > + if (!q) > + return; > + > + if (rtnl_held) > + qdisc_put(q); > + else > + qdisc_put_unlocked(q); > +} This is very ugly. You should know whether RTNL is held or not when calling it. What's more, all of your code passes true, so why do you need a parameter for rtnl_held?