Message-ID: <PH0PR13MB47934158B0D3798B06E2C58194149@PH0PR13MB4793.namprd13.prod.outlook.com>
Date:   Thu, 1 Dec 2022 03:52:23 +0000
From:   Tianyu Yuan <tianyu.yuan@...igine.com>
To:     Marcelo Leitner <mleitner@...hat.com>
CC:     Eelco Chaudron <echaudro@...hat.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Davide Caratti <dcaratti@...hat.com>,
        Edward Cree <edward.cree@....com>,
        Ilya Maximets <i.maximets@....org>,
        Oz Shlomo <ozsh@...dia.com>, Paul Blakey <paulb@...dia.com>,
        Vlad Buslov <vladbu@...dia.com>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        oss-drivers <oss-drivers@...igine.com>,
        Ziyang Chen <ziyang.chen@...igine.com>
Subject: RE: [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE
 when offloading



On Thus, Dec 1 , 2022, at 2:05 AM, Marcelo Leitner wrote:
> 
> On Wed, Nov 30, 2022 at 03:36:57AM +0000, Tianyu Yuan wrote:
> >
> > On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote:
> > >
> > > On 28 Nov 2022, at 14:33, Marcelo Leitner wrote:
> > >
> > > > On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote:
> > > >>
> > > >>
> > > >> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote:
> > > >>
> > > >>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote:
> > > > ...
> > > >>>>
> > > >>>> Furthermore, I think the current stats for each action
> > > >>>> mentioned in
> > > >>>> 2) cannot represent the real hw stats and this is why [ RFC
> > > >>>> net-next v2 0/2] (net: flow_offload: add support for per action
> > > >>>> hw stats)
> > > will come up.
> > > >>>
> > > >>> Exactly. Then, when this patchset (or similar) come up, it won't
> > > >>> update all actions with the same stats anymore. It will require
> > > >>> a set of stats from hw for the gact with PIPE action here. But
> > > >>> if drivers are ignoring this action, they can't have specific
> > > >>> stats for it. Or am I missing something?
> > > >>>
> > > >>> So it is better for the drivers to reject the whole flow instead
> > > >>> of simply ignoring it, and let vswitchd probe if it should or
> > > >>> should not use this action.
> > > >>
> > > >> Please note that OVS does not probe features per interface, but
> > > >> does it
> > > per datapath. So if it’s supported in pipe in tc software, we will
> > > use it. If the driver rejects it, we will probably end up with the tc software
> rule only.
> > > >
> > > > Ah right. I remember it will pick 1 interface for testing and use
> > > > those results everywhere, which then I don't know if it may or may
> > > > not be a representor port or not. Anyhow, then it should use
> > > > skip_sw, to try to probe for the offloading part. Otherwise I'm
> > > > afraid tc sw will always accept this flow and trick the probing, yes.
> > >
> > > Well, it depends on how you look at it. In theory, we should be
> > > hardware agnostic, meaning what if you have different hardware in
> > > your system? OVS only supports global offload enablement.
> > >
> > > Tianyu how are you planning to support this from the OVS side? How
> > > would you probe kernel and/or hardware support for this change?
> >
> > Currently in the test demo, I just extend gact with PIPE (previously
> > only SHOT as default and GOTO_CHAIN when chain exists), and then put
> > such a gact with PIPE at the first place of each filter which will be transacted
> with kernel tc.
> >
> > About the tc sw datapath mentioned, we don't have to make changes
> > because gact with PIPE has already been supported in current tc
> > implementation and it could act like a 'counter' And for the hardware
> > we just need to ignore this PIPE and the stats of this action will still be
> > updated in kernel side and sent to userspace.
> 
> I can't see how the action would have stats from hw if the driver is ignoring
> the action.

The stats for each actions in a filter is updated here in pkt_cls.h:
static inline void
tcf_exts_hw_stats_update(const struct tcf_exts *exts,
			 u64 bytes, u64 packets, u64 drops, u64 lastuse,
			 u8 used_hw_stats, bool used_hw_stats_valid)
{
#ifdef CONFIG_NET_CLS_ACT
	int i;

	for (i = 0; i < exts->nr_actions; i++) {
		struct tc_action *a = exts->actions[i];

		/* if stats from hw, just skip */
		if (tcf_action_update_hw_stats(a)) {
			preempt_disable();
			tcf_action_stats_update(a, bytes, packets, drops,
						lastuse, true);
			preempt_enable();

			a->used_hw_stats = used_hw_stats;
			a->used_hw_stats_valid = used_hw_stats_valid;
		}
	}
#endif
}
In which bytes, packets, drops, lastuse are dumped from the driver, the stats of gact PIPE is updated here is kernel
TC, rather than in driver directly.

> 
> But maybe there was a misunderstanding here. I was reading more the
> cxgb4 driver here and AFAICT this patch will skip PIPE on the action validation,
> but not actually skip the action entirely. Then it will hit
> cxgb4_process_flow_actions() and maybe the driver will the right thing with
> a dummy action out of the blue. Was this your expectation, to just ignore it in
> the validation step, and let it fall through through the driver? If yes, the
> comments are misleading, as the NICs will have to process the packets.
> 

Actually we want to ignore it not only in validation step but also the process step.
We don't want to HW process this action and just want to let the driver treat this flow
with PIPE at the first place as a offloadable one.
> >
> > I agree with that the unsupported actions should be rejected by
> > drivers, so may another approach could work without ignoring PIPE in
> > all the related drivers, that we directly make put the flower stats
> > from driver into the socket which is used to transact with userspace and
>> userspace(e.g. OVS) update the flow stats using this stats instead of the
>> parsing the action stats. How do you think of this?
> 
> I don't understand this approach. Can you please rephrase?
> 

Sorry for not explaining clearly :)
Basically in another approach, we let the cls_fl_filter records the stats for this filter and directly use
this stats to update the flow stats in userspace, the key code changes are following:
......
//update stats in cls_fl_filter:
@@ -500,6 +542,9 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
 			 rtnl_held);
 
+	cls_fl_filter_update_stats(f, cls_flower.stats.bytes,
+				   cls_flower.stats.pkts, true);
+
 	tcf_exts_hw_stats_update(&f->exts, cls_flower.stats.bytes,
 				 cls_flower.stats.pkts,
 				 cls_flower.stats.drops,
//add entries for netlink transaction
@@ -714,6 +759,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_L2TPV3_SID]	= { .type = NLA_U32 },
+	[TCA_FLOWER_STATS]	= { .type = NLA_NESTED},
+	[TCA_FLOWER_LASTUSE]	= { .type = NLA_U64 },
 
 };
......
//copy stats in cls_fl_filter into the socket which will be sent to userspace
@@ -3319,6 +3392,9 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
 	if (tcf_exts_dump(skb, &f->exts))
 		goto nla_put_failure;
 
+	if (tcf_filter_copy_stats(skb, f) < 0)
+		goto nla_put_failure;
+
 	nla_nest_end(skb, nest);
 
 	if (tcf_exts_dump_stats(skb, &f->exts) < 0)
@@ -3364,6 +3440,9 @@ static int fl_terse_dump(struct net *net, struct tcf_proto *tp, void *fh,
 	if (tcf_exts_terse_dump(skb, &f->exts))
 		goto nla_put_failure;
 
+	if (tcf_filter_copy_stats(skb, f) < 0)
+		goto nla_put_failure;
+
 	nla_nest_end(skb, nest);
 
 	return skb->len;
......

In this approach we don't have to make changes in drivers and they validate and process
flows as before.
I hope this could make it clear and if there is something wrong or misunderstanding, please
let me know.

Thanks,
Tianyu

> Thanks,
> Marcelo
> 
> >
> > Cheers,
> > Tianyu
> > >
> > > //Eelco
> >

Powered by blists - more mailing lists