Re: [PATCH net v6 1/1] net/sched: act_gate: snapshot parameters with RCU on replace

[Paul Moses]

1. hrtimer_get_expires() just returns the stored node.expires and
   hrtimer_cancel() doesn’t clear it, so expires==0 is not a reliable
   inactivity test. Logic was that although I detected no observable
   behavior difference, relying on stale expires could theoretically
   cause infrequent subtle intermittent misses of intended behavior.
   It's maybe more appropriate to leave it alone for stable or at
   least not in this patch/series?

2. Agreed. This was a mistake.

3. It's the same pattern used in sch_taprio and it's documented in
   Documentation/memory-barriers.txt: the compiler may merge/discard/
   invent/reorder plain accesses and READ_ONCE()/WRITE_ONCE() exist to
   make intentional lockless shared variable accesses well defined.
   Since tk_offset is read with READ_ONCE() outside tcf_lock, the writer
   uses WRITE_ONCE() to pair with that lockless read.

4. Agreed, I’ll remove the redundant guard.

5. goto_ch is initialized to NULL and tcf_action_check_ctrlact() only sets
   it on success, so the current code is safe, but I agree that it's confusing,
   I'll improve.

Thanks
Paul



On Sunday, February 15th, 2026 at 2:45 PM, Victor Nogueira <victor@xxxxxxxxxxxx> wrote:

> On Fri, Feb 13, 2026 at 8:39 AM Paul Moses <p@xxxxxxx> wrote:
> >
> > The gate action can be replaced while the hrtimer callback or dump path is
> > walking the schedule list.
> >
> > Convert the parameters to an RCU-protected snapshot and swap updates under
> > tcf_lock, freeing the previous snapshot via call_rcu(). When REPLACE omits
> > the entry list, preserve the existing schedule so the effective state is
> > unchanged.
> > [...]
> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > index c1f75f2727576..60c80e609ec3d 100644
> > --- a/net/sched/act_gate.c
> > +++ b/net/sched/act_gate.c
> > [...]
> > @@ -56,11 +59,10 @@ static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
> >  {
> >         ktime_t expires;
> >
> > -       expires = hrtimer_get_expires(&gact->hitimer);
> > -       if (expires == 0)
> > -               expires = KTIME_MAX;
> > -
> > -       start = min_t(ktime_t, start, expires);
> > +       if (hrtimer_active(&gact->hitimer)) {
> > +               expires = hrtimer_get_expires(&gact->hitimer);
> > +               start = min_t(ktime_t, start, expires);
> > +       }
> 
> Is this change really necessary?
> 
> > [...]
> >  static int parse_gate_list(struct nlattr *list_attr,
> >                            struct tcf_gate_params *sched,
> >                            struct netlink_ext_ack *extack)
> > @@ -261,7 +294,6 @@ static int parse_gate_list(struct nlattr *list_attr,
> >         }
> >
> >         sched->num_entries = i;
> > -
> >         return i;
> 
> Removing this line also seems unnecessary.
> 
> > [...]
> > +static void gate_setup_timer(struct tcf_gate *gact, s32 clockid,
> > +                            enum tk_offsets tko)
> > +{
> > +       WRITE_ONCE(gact->tk_offset, tko);
> 
> Why do you need this WRITE_ONCE?
> 
> >  static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > [...]
> > @@ -366,6 +407,60 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > [...]
> > +       if (ret != ACT_P_CREATED) {
> > [...]
> > +               if (use_old_entries) {
> > +                       err = tcf_gate_copy_entries(p, cur_p, extack);
> > +                       if (!err && !tb[TCA_GATE_CYCLE_TIME])
> 
> This check for TCA_GATE_CYCLE_TIME seems unnecessary.
> If I understand your code correctly, cycletime will be overwritten
> further down if TCA_GATE_CYCLE_TIME was specified.
> 
> > +                               cycletime = cur_p->tcfg_cycletime;
> > [...]
> > @@ -434,33 +532,47 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > [...]
> > -chain_put:
> > +unlock:
> >         spin_unlock_bh(&gact->tcf_lock);
> >
> > +err_free:
> > +       release_entry_list(&p->entries);
> > +       kfree(p);
> > +release_idr:
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > -release_idr:
> > [...]
> 
> This looks weird.
> You will go to the release_idr label when tcf_action_check_ctrlact fails,
> so the "if (goto_ch)" part of the code will be reached in that code path.
> I believe it would be better to keep the "chain_put" label and keep
> "release_idr" below it (as it was before your change).
> Something like:
> 
> chain_put:
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> release_idr:
>         ...
> 
> cheers,
> Victor
>