Re: [patch net-next v3 05/10] net: sched: keep track of offloaded filters and check tc offload feature

[Jiri Pirko <jiri@resnulli.us>]

Thu, Dec 14, 2017 at 02:05:55AM CET, jakub.kicinski@netronome.com wrote:
>On Wed, 13 Dec 2017 16:10:33 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> During block bind, we need to check tc offload feature. If it is
>> disabled yet still the block contains offloaded filters, forbid the
>> bind. Also forbid to register callback for a block that already
>> containes offloaded filters, as the play back is not supported now.
>> For keeping track of offloaded filters there is a new counter
>> introduced, alongside with couple of helpers called from cls_* code.
>> These helpers set and clear TCA_CLS_FLAGS_IN_HW flag.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index de9dbcb..ac25142 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -266,31 +266,50 @@ void tcf_chain_put(struct tcf_chain *chain)
>>  }
>>  EXPORT_SYMBOL(tcf_chain_put);
>>  
>> -static void tcf_block_offload_cmd(struct tcf_block *block, struct Qdisc *q,
>> +static bool tcf_block_offload_in_use(struct tcf_block *block)
>> +{
>> +	return block->offloadcnt;
>> +}
>> +
>> +static void tcf_block_offload_cmd(struct tcf_block *block,
>> +				  struct net_device *dev,
>>  				  struct tcf_block_ext_info *ei,
>>  				  enum tc_block_command command)
>>  {
>> -	struct net_device *dev = q->dev_queue->dev;
>>  	struct tc_block_offload bo = {};
>>  
>> -	if (!dev->netdev_ops->ndo_setup_tc)
>> -		return;
>>  	bo.command = command;
>>  	bo.binder_type = ei->binder_type;
>>  	bo.block = block;
>>  	dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
>>  }
>>  
>> -static void tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>> -				   struct tcf_block_ext_info *ei)
>> +static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>> +				  struct tcf_block_ext_info *ei)
>>  {
>> -	tcf_block_offload_cmd(block, q, ei, TC_BLOCK_BIND);
>> +	struct net_device *dev = q->dev_queue->dev;
>> +
>> +	if (!dev->netdev_ops->ndo_setup_tc)
>> +		return 0;
>> +
>> +	/* If tc offload feature is disabled and the block we try to bind
>> +	 * to already has some offloaded filters, forbid to bind.
>> +	 */
>> +	if (!tc_can_offload(dev) && tcf_block_offload_in_use(block))
>> +		return -EOPNOTSUPP;
>
>I don't understand the tc_can_offload(dev) check here.  The flow is -
>on bind if TC offloads are enabled the new port will get a TC_BLOCK_BIND
>and request a register, but the register will fail since the block is
>offloaded?

The point of this check is to disallow dev with tc offload disabled to
share a block which already holds offloaded filters.

That is similar to disallow disabling tc offload on device that shares a
block which contains offloaded filters.



>
>But the whole bind operation does not fail, so user will not see an
>error.  The block will get bound but port's driver has no way to
>register the callback.  I'm sorry if I'm just being slow here..
>
>> +	tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND);
>> +	return 0;

The thing is that driver which does not support TC_BLOCK_BIND would
return -EOPNOTSUPP here. For those drivers we continue, they just won't
have block cb registered so they won't receive cb calls to offload
filters.


>>  }
>>  
>>  static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
>>  				     struct tcf_block_ext_info *ei)
>>  {
>> -	tcf_block_offload_cmd(block, q, ei, TC_BLOCK_UNBIND);
>> +	struct net_device *dev = q->dev_queue->dev;
>> +
>> +	if (!dev->netdev_ops->ndo_setup_tc)
>> +		return;
>> +	tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_UNBIND);
>>  }
>>  
>>  static int
>> @@ -499,10 +518,15 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
>>  	if (err)
>>  		goto err_chain_head_change_cb_add;
>>  
>> -	tcf_block_offload_bind(block, q, ei);
>> +	err = tcf_block_offload_bind(block, q, ei);
>> +	if (err)
>> +		goto err_block_offload_bind;
>
>Would it perhaps make more sense to add a TC_BLOCK_JOIN or some such?

Why? Just a namechange?


>IIUC the problem is we don't know whether the driver/callee of the new
>port is aware of previous callbacks/filters and we can't replay them.
>
>Obviously registering new callbacks on offloaded blocks is a no-go.
>For simple bind to a new port of an ASIC which already knows the rule
>set, we just need to ask all callbacks if they know the port and as
>long as any of them responds "yes" we are safe to assume the bind is OK.
>
>(Existing drivers will all respond with EOPNOTSUPP to a new unknown command.)
>
>Does that make sense?

Hmm, I understand what you say. I have to think about that a bit more.

Thanks!

>
>>  	*p_block = block;
>>  	return 0;
>>  
>> +err_block_offload_bind:
>> +	tcf_chain_head_change_cb_del(tcf_block_chain_zero(block), ei);
>>  err_chain_head_change_cb_add:
>>  	tcf_block_owner_del(block, q, ei->binder_type);
>>  err_block_owner_add:
>> @@ -630,9 +654,16 @@ struct tcf_block_cb *__tcf_block_cb_register(struct tcf_block *block,
>>  {
>>  	struct tcf_block_cb *block_cb;
>>  
>> +	/* At this point, playback of previous block cb calls is not supported,
>> +	 * so forbid to register to block which already has some offloaded
>> +	 * filters present.
>> +	 */
>> +	if (tcf_block_offload_in_use(block))
>> +		return ERR_PTR(-EOPNOTSUPP);
>> 
>>  	block_cb = kzalloc(sizeof(*block_cb), GFP_KERNEL);
>>  	if (!block_cb)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>  	block_cb->cb = cb;
>>  	block_cb->cb_ident = cb_ident;
>>  	block_cb->cb_priv = cb_priv;
>> @@ -648,7 +679,7 @@ int tcf_block_cb_register(struct tcf_block *block,
>>  	struct tcf_block_cb *block_cb;
>>  
>>  	block_cb = __tcf_block_cb_register(block, cb, cb_ident, cb_priv);
>> -	return block_cb ? 0 : -ENOMEM;
>> +	return IS_ERR(block_cb) ? PTR_ERR(block_cb) : 0;
>>  }
>>  EXPORT_SYMBOL(tcf_block_cb_register);
>>