On Mon, Aug 20, 2018 at 11:29 AM David Miller <da...@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangc...@gmail.com>
> Date: Sun, 19 Aug 2018 12:22:12 -0700
>
> > The only time we need to take tcfa_lock is when adding
> > a new metainfo to an existing ife->metalist. We don't need
> > to take tcfa_lock so early and so broadly in tcf_ife_init().
> >
> > This means we can always take ife_mod_lock first, avoid the
> > reverse locking ordering warning as reported by Vlad.
> >
> > Reported-by: Vlad Buslov <vla...@mellanox.com>
> > Tested-by: Vlad Buslov <vla...@mellanox.com>
> > Cc: Vlad Buslov <vla...@mellanox.com>
> > Cc: Jamal Hadi Salim <j...@mojatatu.com>
> > Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
>
> After this change we no longer call populate_metalist() in an atomic
> context via tcf_ife_init(), and populate_metalist passes 'exists'
> down to add_metainfo() as an 'atomic' indicator. It doesn't have this
> meaning if you aren't holding the tcfa_lock in the callers with BH
> disabled.
Passing 'exists' as 'atomic' is prior to my change. With my change,
they are separated as two parameters:
static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
- int len, bool atomic)
+ int len, bool atomic, bool exists)
Or I misunderstand your point here?
>
> Therefore, add_metainfo()'s 'atomic' indication is inaccurate in this
> call chain and will use GFP_ATOMIC unnecessarily.
>
> Probably the thing to just is just pass 'false' down to add_metainfo()
> in populate_metalist().
This is exactly what this patch does?
static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
bool exists, bool rtnl_held)
{
@@ -436,12 +431,11 @@ static int populate_metalist(struct tcf_ife_info
*ife, struct nlattr **tb,
...
- rc = add_metainfo(ife, i, val, len, exists);
+ rc = add_metainfo(ife, i, val, len, false, exists);
