Re: TC stats / hw offload question

Edward Cree
2019-04-29T16:25:19Z

On 29/04/2019 16:21, Pablo Neira Ayuso wrote:
> On Mon, Apr 29, 2019 at 03:11:06PM +0100, Edward Cree wrote:
>> This is a bit of a mess; the best idea I've got is for the
>>  TC_CLSFLOWER_STATS call to include a tcfa_index.  Then the driver
>>  returns counter stats for that index, and tcf_exts_stats_update()
>>  only updates those actions whose index matches.  But then
>>  fl_hw_update_stats() would have to iterate over all the indices in
>>  f->exts.  What do you think?
> You could extend struct flow_stats to pass an array of stats to the
> driver, including one stats per action and the counter index. Then,
> tcf_exts_stats_update() uses this array of stats to update per-action
> stats.
Yes, but that means allocating the flow_stats.stats array each time;
I'd rather avoid memory allocation unless it's necessary.  As long as
we can move the preempt_disable() inside the loop that's currently in
tcf_exts_stats_update() (i.e. only disable pre-emption across each
individual call to tcf_action_stats_update()) I think we can.
I think I prefer my approach (ask for one tcfa_index at a time); but
unmodified drivers that don't look at the passed index would return
zeroes for actions after the first, so we'll need some way to handle
those drivers separately (e.g. one tc_setup_cb_call with "answer
this one if you don't do indices" and a bunch more with specified
index values).  I think that requires much less change to the
existing drivers than putting an array back in the API, and keeps as
much of the work as possible in the core where it won't have to be
replicated in every driver.
I'll put an RFC patch together soonish if no objections.

-Ed