Re: TC stats / hw offload question

[Edward Cree <ecree@solarflare.com>]

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

-Ed

[1]: https://www.spinics.net/lists/netdev/msg551448.html