Message-ID: <4553b684-56ec-fe81-1692-b11e10914941@gmail.com>
Date:   Wed, 15 Mar 2023 14:43:26 +0000
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        edward.cree@....com
Cc:     linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, netdev@...r.kernel.org,
        habetsm.xilinx@...il.com
Subject: Re: [PATCH net-next 5/5] sfc: add offloading of 'foreign' TC (decap)
 rules

On 15/03/2023 10:11, Michal Swiatkowski wrote:
> On Tue, Mar 14, 2023 at 05:35:25PM +0000, edward.cree@....com wrote:
>> +int efx_mae_check_encap_type_supported(struct efx_nic *efx, enum efx_encap_type typ)
>> +{
>> +	unsigned int bit;
>> +
>> +	switch (typ & EFX_ENCAP_TYPES_MASK) {
> In 3/5 You ommited EFX_ENCAP_TYPE_NVGRE, was it intetional?

No, I'll go back and add it.

>> +enum efx_encap_type efx_tc_indr_netdev_type(struct net_device *net_dev)
>> +{
>> +	if (netif_is_vxlan(net_dev))
>> +		return EFX_ENCAP_TYPE_VXLAN;
>> +	if (netif_is_geneve(net_dev))
>> +		return EFX_ENCAP_TYPE_GENEVE;
> netif_is_gretap or NVGRE isn't supported?

It should be supported, the hardware can handle it.
I'll add it in v2, and test to make sure it actually works ;)

>> +static const char *efx_tc_encap_type_name(enum efx_encap_type typ, char *buf,
>> +					  size_t n)
>> +{
>> +	switch (typ) {
>> +	case EFX_ENCAP_TYPE_NONE:
>> +		return "none";
>> +	case EFX_ENCAP_TYPE_VXLAN:
>> +		return "vxlan";
>> +	case EFX_ENCAP_TYPE_NVGRE:
>> +		return "nvgre";
>> +	case EFX_ENCAP_TYPE_GENEVE:
>> +		return "geneve";
>> +	default:
>> +		snprintf(buf, n, "type %u\n", typ);
>> +		return buf;
> I will return unsupported here, instead of playing with buffer.

Hmm, maybe if I add a one-time netif_warn()?  I don't like the
 idea of not getting the bogus value out anywhere where it can
 be debugged.

>>  /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
> Is it a device documentation? Where it can be find?

Ahh, turns out this document isn't public :(  I'll see if we
 can get this section of it split out into something public.

> 
>>  enum efx_tc_action_order {
>> +	EFX_TC_AO_DECAP,
>>  	EFX_TC_AO_VLAN_POP,
>>  	EFX_TC_AO_VLAN_PUSH,
>>  	EFX_TC_AO_COUNT,
>> @@ -513,6 +558,10 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>>  					  enum efx_tc_action_order new)
>>  {
>>  	switch (new) {
>> +	case EFX_TC_AO_DECAP:
>> +		if (act->decap)
>> +			return false;
>> +		fallthrough;
>>  	case EFX_TC_AO_VLAN_POP:
>>  		if (act->vlan_pop >= 2)
>>  			return false;
>> @@ -540,6 +589,288 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
>>  	}
>>  }
>>  
>> +static int efx_tc_flower_replace_foreign(struct efx_nic *efx,
>> +					 struct net_device *net_dev,
>> +					 struct flow_cls_offload *tc)
>> +{
>> +	struct flow_rule *fr = flow_cls_offload_flow_rule(tc);
>> +	struct netlink_ext_ack *extack = tc->common.extack;
>> +	struct efx_tc_flow_rule *rule = NULL, *old = NULL;
>> +	struct efx_tc_action_set *act = NULL;
>> +	bool found = false, uplinked = false;
>> +	const struct flow_action_entry *fa;
>> +	struct efx_tc_match match;
>> +	struct efx_rep *to_efv;
>> +	s64 rc;
>> +	int i;
>> +
>> +	/* Parse match */
>> +	memset(&match, 0, sizeof(match));
>> +	rc = efx_tc_flower_parse_match(efx, fr, &match, NULL);
>> +	if (rc)
>> +		return rc;
>> +	/* The rule as given to us doesn't specify a source netdevice.
>> +	 * But, determining whether packets from a VF should match it is
>> +	 * complicated, so leave those to the software slowpath: qualify
>> +	 * the filter with source m-port == wire.
>> +	 */
>> +	rc = efx_tc_flower_external_mport(efx, EFX_EFV_PF);
> Let's define extern_port as s64, a rc as int, it will be more readable I
> think.

Will it?  I feel it looks more natural this way — this is the way
 it would be written if mports always fitted into an int; the fact
 that they're actually u32 and thus the value needs to be wider to
 hold either an mport or an -errno is locally irrelevant to the
 reader.

>> +	/* Parse actions.  For foreign rules we only support decap & redirect */
>> +	flow_action_for_each(i, fa, &fr->action) {
>> +		struct efx_tc_action_set save;
>> +
>> +		switch (fa->id) {
>> +		case FLOW_ACTION_REDIRECT:
>> +		case FLOW_ACTION_MIRRED:
>> +			/* See corresponding code in efx_tc_flower_replace() for
>> +			 * long explanations of what's going on here.
>> +			 */
>> +			save = *act;
> Why save is needed here? In one bloick You are changing act, in other
> save.

Below, we set:

>> +			act->dest_mport = rc;
>> +			act->deliver = 1;

but then if this action is a mirred egress mirror, we don't want the
 next action-set in the action-set-list to deliver to the same place.
So we save the action state (collection of mutations applied to the
 packet) before the mirred, so that we can resume from where we left
 off:

>> +			/* Mirror, so continue on with saved act */
>> +			save.count = NULL;
>> +			act = kzalloc(sizeof(*act), GFP_USER);
>> +			if (!act) {
>> +				rc = -ENOMEM;
>> +				goto release;
>> +			}
>> +			*act = save;

The setting of save.count is a copy-paste error; that's needed in
 the non-tunnel case (efx_tc_flower_replace()) since that has a
 separate block for attaching counters (handling the case of
 FLOW_ACTION_DROP which we don't accept here in
 efx_tc_flower_replace_foreign()), but it's not needed here; maybe
 that was part of the confusion.

>> +			rc = efx_mae_alloc_action_set(efx, act);
>> +			if (rc) {
>> +				NL_SET_ERR_MSG_MOD(extack,
>> +						   "Failed to write action set to hw (mirred)");
>> +				goto release;
>> +			}
>> +			list_add_tail(&act->list, &rule->acts.list);
>> +			act = NULL;
> act was allocated, You need to free it, or maybe it is being cleared in
> alloc_action_set()? However, this function is really hard to follow.
> Please explain it more widely.

The list_add_tail attaches act (the action-set) to the list in
 rule->acts (the action-set-list), meaning it will be freed when the
 action-set-list is destroyed by efx_tc_free_action_set_list(),
 either in this function's failure ladder or in efx_tc_delete_rule().
I will try and write an extended comment under /* Parse actions */ to
 explain the use of this act 'cursor'.  Or rather, I'll add the
 comment to the efx_tc_flower_replace() equivalent, which works the
 same way, and reference it from here.

>> +	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
>> +				 rule->acts.fw_id, &rule->fw_id);
>> +	if (rc) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
>> +		goto release_act;
>> +	}
> act is saved somewhere?

Sorry, this should be called `release_acts`, as its job is to
 remove the action-set-list (rule->acts) from hardware.  Will fix.

Thanks for the very thorough review of this series :)

Powered by blists - more mailing lists