Re: TC stats / hw offload question

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Apr 26, 2019 at 01:13:41PM +0100, Edward Cree wrote:
> On 25/04/2019 23:33, Pablo Neira Ayuso wrote:
> > On Thu, Apr 25, 2019 at 02:23:08PM +0100, Edward Cree wrote:
> >> On 24/04/2019 16:03, Edward Cree wrote:
> >>> static int efx_tc_flower_replace(struct efx_nic *efx,
> >>>                                  struct net_device *net_dev,
> >>>                                  struct tc_cls_flower_offload *tc)
> >>> {
> >>>     struct efx_tc_action_set *act;
> >>>
> >>>     /* parse the match */
> >>>
> >>>     tcf_exts_for_each_action(i, a, tc->exts) {
> >>>         if (a->ops && a->ops->stats_update) {
> >>>             /* act is the hw action we're building */
> >>>             act->count = allocate_a_counter();
> >> Also, this was actually taking a->tcfa_index, allowing multiple rules to
> >>  share a counter.  The action index doesn't seem to be available in the
> >>  new flow_offload API.
> > Could you show a bit more code to see how you use a->tcfa_index from
> > your efx_tc_flower_replace()?
> >
> > Thanks.
> Sure; this block is (still slightly abridged)
> 
> if (a->ops && a->ops->stats_update) {
>     struct efx_tc_counter_index *ctr;
> 
>     ctr = efx_tc_flower_get_counter_by_index(efx, a->tcfa_index);
>     if (IS_ERR(ctr)) {
>         rc = PTR_ERR(ctr);
>         goto release;
>     }
>     act->count = ctr;
>     act->count_action_idx = i;
>     efx_tc_calculate_count_delta(act);
> }
> 
> and we have
> 
> struct efx_tc_counter_index {
>     u32 tcfa_index;
>     struct rhash_head linkage;
>     refcount_t ref;
>     u32 fw_id;
> };
> 
> const static struct rhashtable_params efx_tc_counter_ht_params = {
>     .key_len        = offsetof(struct efx_tc_counter_index, linkage),
>     .key_offset     = 0,
>     .head_offset    = offsetof(struct efx_tc_counter_index, linkage),
> };
> 
> static struct efx_tc_counter_index *efx_tc_flower_get_counter_by_index(
>                 struct efx_nic *efx, u32 idx)
> {
>     struct efx_tc_counter_index *ctr, *old;
>     long rc;
> 
>     ctr = kzalloc(sizeof(*ctr), GFP_USER);
>     if (!ctr)
>         return ERR_PTR(-ENOMEM);
>     ctr->tcfa_index = idx;
>     old = rhashtable_lookup_get_insert_fast(&efx->tc->counter_ht,
>                                             &ctr->linkage,
>                                             efx_tc_counter_ht_params);
>     if (old) {
>         /* don't need our new entry */
>         kfree(ctr);
>         if (!refcount_inc_not_zero(&old->ref))
>             return ERR_PTR(-EAGAIN);
>         /* existing entry found */
>         ctr = old;
>     } else {
>         rc = efx_tc_flower_allocate_counter(efx);
>         if (rc < 0) {
>             rhashtable_remove_fast(&efx->tc->counter_ht,
>                                    &ctr->linkage,
>                                    efx_tc_counter_ht_params);
>             kfree(ctr);
>             return ERR_PTR(rc);
>         }
>         ctr->fw_id = rc;
>         refcount_inc(&ctr->ref);
>     }
>     return ctr;
> }
> 
> Thus if (and only if) two TC actions have the same tcfa_index, they will
>  share a single counter in the HW.
> I gathered from a previous conversation with Jamal[1] that that was the
>  correct behaviour:
> > Note, your counters should also be shareable; example, count all
> > the drops in one counter across multiple flows as in the following
> > case where counter index 1 is used.
> >
> > tc flower match foo action drop index 1
> > tc flower match bar action drop index 1

The flow_action_entry structure needs a new 'counter_index' field to
store this. The tc_setup_flow_action() function needs to be updated
for this for the FLOW_ACTION_{ACCEPT,DROP,REDIRECT,MIRRED} cases to
set this entry->counter_index field to tcfa_index, so the driver has
access to this.

Thanks.