| Message ID | 20240215160458.1727237-2-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | make skip_sw actually skip software | expand |
+Cc Vlad and Marcelo.. On Thu, Feb 15, 2024 at 11:06 AM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote: > > Maintain a count of skip_sw filters. > > This counter is protected by the cb_lock, and is updated > at the same time as offloadcnt. > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > --- > include/net/sch_generic.h | 1 + > net/sched/cls_api.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > index 934fdb977551..46a63d1818a0 100644 > --- a/include/net/sch_generic.h > +++ b/include/net/sch_generic.h > @@ -476,6 +476,7 @@ struct tcf_block { > struct flow_block flow_block; > struct list_head owner_list; > bool keep_dst; > + atomic_t skipswcnt; /* Number of skip_sw filters */ > atomic_t offloadcnt; /* Number of oddloaded filters */ For your use case is skipswcnt ever going to be any different than offloadcnt? cheers, jamal > unsigned int nooffloaddevcnt; /* Number of devs unable to do offload */ > unsigned int lockeddevcnt; /* Number of devs that require rtnl lock. */ > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index ca5676b2668e..397c3d29659c 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -3483,6 +3483,8 @@ static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags) > if (*flags & TCA_CLS_FLAGS_IN_HW) > return; > *flags |= TCA_CLS_FLAGS_IN_HW; > + if (tc_skip_sw(*flags)) > + atomic_inc(&block->skipswcnt); > atomic_inc(&block->offloadcnt); > } > > @@ -3491,6 +3493,8 @@ static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags) > if (!(*flags & TCA_CLS_FLAGS_IN_HW)) > return; > *flags &= ~TCA_CLS_FLAGS_IN_HW; > + if (tc_skip_sw(*flags)) > + atomic_dec(&block->skipswcnt); > atomic_dec(&block->offloadcnt); > } > > -- > 2.43.0 >
Hi Jamal, Thank you for the review. On 2/15/24 17:39, Jamal Hadi Salim wrote: > +Cc Vlad and Marcelo.. > > On Thu, Feb 15, 2024 at 11:06 AM Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote: >> >> Maintain a count of skip_sw filters. >> >> This counter is protected by the cb_lock, and is updated >> at the same time as offloadcnt. >> >> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >> --- >> include/net/sch_generic.h | 1 + >> net/sched/cls_api.c | 4 ++++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >> index 934fdb977551..46a63d1818a0 100644 >> --- a/include/net/sch_generic.h >> +++ b/include/net/sch_generic.h >> @@ -476,6 +476,7 @@ struct tcf_block { >> struct flow_block flow_block; >> struct list_head owner_list; >> bool keep_dst; >> + atomic_t skipswcnt; /* Number of skip_sw filters */ >> atomic_t offloadcnt; /* Number of oddloaded filters */ > > For your use case is skipswcnt ever going to be any different than offloadcnt? No, we only use skip_sw filters, since we only use TC as a control path to install skip_sw rules into hardware. AFAICT offloadcnt is the sum of skip_sw filters, and filters with no flags which have implicitly been offloaded. The reason that I didn't just use offloadcnt, is that I'm not sure if it is acceptable to treat implicitly offloaded rules without skip_sw, as if they were explicitly skip_sw. It sounds reasonable, given that the filters without skip_* flags shouldn't really care. I tried to only trigger the TC bypass, in the cases that I was absolutely sure would be safe as a first step. > > cheers, > jamal > >> unsigned int nooffloaddevcnt; /* Number of devs unable to do offload */ >> unsigned int lockeddevcnt; /* Number of devs that require rtnl lock. */ >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index ca5676b2668e..397c3d29659c 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -3483,6 +3483,8 @@ static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags) >> if (*flags & TCA_CLS_FLAGS_IN_HW) >> return; >> *flags |= TCA_CLS_FLAGS_IN_HW; >> + if (tc_skip_sw(*flags)) >> + atomic_inc(&block->skipswcnt); >> atomic_inc(&block->offloadcnt); >> } >> >> @@ -3491,6 +3493,8 @@ static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags) >> if (!(*flags & TCA_CLS_FLAGS_IN_HW)) >> return; >> *flags &= ~TCA_CLS_FLAGS_IN_HW; >> + if (tc_skip_sw(*flags)) >> + atomic_dec(&block->skipswcnt); >> atomic_dec(&block->offloadcnt); >> } >> >> -- >> 2.43.0 >>
On Thu 15 Feb 2024 at 23:34, Asbjørn Sloth Tønnesen <ast@fiberby.net> wrote: > Hi Jamal, > > Thank you for the review. > > On 2/15/24 17:39, Jamal Hadi Salim wrote: >> +Cc Vlad and Marcelo.. >> On Thu, Feb 15, 2024 at 11:06 AM Asbjørn Sloth Tønnesen <ast@fiberby.net> >> wrote: >>> >>> Maintain a count of skip_sw filters. >>> >>> This counter is protected by the cb_lock, and is updated >>> at the same time as offloadcnt. >>> >>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >>> --- >>> include/net/sch_generic.h | 1 + >>> net/sched/cls_api.c | 4 ++++ >>> 2 files changed, 5 insertions(+) >>> >>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >>> index 934fdb977551..46a63d1818a0 100644 >>> --- a/include/net/sch_generic.h >>> +++ b/include/net/sch_generic.h >>> @@ -476,6 +476,7 @@ struct tcf_block { >>> struct flow_block flow_block; >>> struct list_head owner_list; >>> bool keep_dst; >>> + atomic_t skipswcnt; /* Number of skip_sw filters */ >>> atomic_t offloadcnt; /* Number of oddloaded filters */ >> For your use case is skipswcnt ever going to be any different than offloadcnt? > > No, we only use skip_sw filters, since we only use TC as a control path to > install skip_sw rules into hardware. > > AFAICT offloadcnt is the sum of skip_sw filters, and filters with no flags which > have implicitly been offloaded. > > The reason that I didn't just use offloadcnt, is that I'm not sure if it is > acceptable to treat implicitly offloaded rules without skip_sw, as if they were > explicitly skip_sw. It sounds reasonable, given that the filters without skip_* flags > shouldn't really care. It is not acceptable since there are valid use-cases where packets need to match sw filters that are supposedly also in-hw. For example, filters with tunnel_key set action during neighbor update event. > > I tried to only trigger the TC bypass, in the cases that I was absolutely sure would > be safe as a first step. > > >> cheers, >> jamal >> >>> unsigned int nooffloaddevcnt; /* Number of devs unable to do offload */ >>> unsigned int lockeddevcnt; /* Number of devs that require rtnl lock. */ >>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >>> index ca5676b2668e..397c3d29659c 100644 >>> --- a/net/sched/cls_api.c >>> +++ b/net/sched/cls_api.c >>> @@ -3483,6 +3483,8 @@ static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags) >>> if (*flags & TCA_CLS_FLAGS_IN_HW) >>> return; >>> *flags |= TCA_CLS_FLAGS_IN_HW; >>> + if (tc_skip_sw(*flags)) >>> + atomic_inc(&block->skipswcnt); >>> atomic_inc(&block->offloadcnt); >>> } >>> >>> @@ -3491,6 +3493,8 @@ static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags) >>> if (!(*flags & TCA_CLS_FLAGS_IN_HW)) >>> return; >>> *flags &= ~TCA_CLS_FLAGS_IN_HW; >>> + if (tc_skip_sw(*flags)) >>> + atomic_dec(&block->skipswcnt); >>> atomic_dec(&block->offloadcnt); >>> } >>> >>> -- >>> 2.43.0 >>>
Thu, Feb 15, 2024 at 05:04:42PM CET, ast@fiberby.net wrote: >Maintain a count of skip_sw filters. > >This counter is protected by the cb_lock, and is updated >at the same time as offloadcnt. > >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 934fdb977551..46a63d1818a0 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -476,6 +476,7 @@ struct tcf_block { struct flow_block flow_block; struct list_head owner_list; bool keep_dst; + atomic_t skipswcnt; /* Number of skip_sw filters */ atomic_t offloadcnt; /* Number of oddloaded filters */ unsigned int nooffloaddevcnt; /* Number of devs unable to do offload */ unsigned int lockeddevcnt; /* Number of devs that require rtnl lock. */ diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index ca5676b2668e..397c3d29659c 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3483,6 +3483,8 @@ static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags) if (*flags & TCA_CLS_FLAGS_IN_HW) return; *flags |= TCA_CLS_FLAGS_IN_HW; + if (tc_skip_sw(*flags)) + atomic_inc(&block->skipswcnt); atomic_inc(&block->offloadcnt); } @@ -3491,6 +3493,8 @@ static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags) if (!(*flags & TCA_CLS_FLAGS_IN_HW)) return; *flags &= ~TCA_CLS_FLAGS_IN_HW; + if (tc_skip_sw(*flags)) + atomic_dec(&block->skipswcnt); atomic_dec(&block->offloadcnt); }
Maintain a count of skip_sw filters. This counter is protected by the cb_lock, and is updated at the same time as offloadcnt. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- include/net/sch_generic.h | 1 + net/sched/cls_api.c | 4 ++++ 2 files changed, 5 insertions(+)