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
