> On Apr 25, 2019, at 12:02 AM, Ori Kam <or...@mellanox.com> wrote:
>
> Hi Yongseok,
> PSB,
>
> Ori
>
>> -----Original Message-----
>> From: Yongseok Koh
>> Sent: Wednesday, April 24, 2019 12:03 AM
>> To: Ori Kam <or...@mellanox.com>
>> Cc: Shahaf Shuler <shah...@mellanox.com>; Matan Azrad
>> <ma...@mellanox.com>; dev@dpdk.org
>> Subject: Re: [PATCH] net/mlx5: fix E-Switch flow without port item
>>
>> On Tue, Apr 23, 2019 at 11:19:16AM +0000, Ori Kam wrote:
>>> When creating a flow rule without the port_id pattern item, always the
>>> PF was selected.
>>>
>>> This commit fixes this issue, if no port_id pattern item is available
>>> then we use the port that the flow was created on as source port.
>>>
>>> Fixes: 822fb3195348 ("net/mlx5: add port id item to Direct Verbs")
>>>
>>> Signed-off-by: Ori Kam <or...@mellanox.com>
>>> ---
>>> drivers/net/mlx5/mlx5_flow_dv.c | 30 +++++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
>> b/drivers/net/mlx5/mlx5_flow_dv.c
>>> index c2a2fc6..d17adbe 100644
>>> --- a/drivers/net/mlx5/mlx5_flow_dv.c
>>> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
>>> @@ -3623,6 +3623,8 @@ struct field_modify_info modify_tcp[] = {
>>> union flow_dv_attr flow_attr = { .attr = 0 };
>>> struct mlx5_flow_dv_tag_resource tag_resource;
>>> uint32_t modify_action_position = UINT32_MAX;
>>> + void *match_mask = matcher.mask.buf;
>>> + void *match_value = dev_flow->dv.value.buf;
>>>
>>> flow->group = attr->group;
>>> if (attr->transfer)
>>> @@ -3895,23 +3897,8 @@ struct field_modify_info modify_tcp[] = {
>>> }
>>> dev_flow->dv.actions_n = actions_n;
>>> flow->actions = action_flags;
>>> - if (attr->ingress && !attr->transfer &&
>>> - (priv->representor || priv->master)) {
>>> - /* It was validated - we support unidirection flows only. */
>>> - assert(!attr->egress);
>>> - /*
>>> - * Add matching on source vport index only
>>> - * for ingress rules in E-Switch configurations.
>>> - */
>>> - flow_dv_translate_item_source_vport(matcher.mask.buf,
>>> - dev_flow->dv.value.buf,
>>> - priv->vport_id,
>>> - 0xffff);
>>> - }
>>> for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
>>> int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
>>> - void *match_mask = matcher.mask.buf;
>>> - void *match_value = dev_flow->dv.value.buf;
>>>
>>> switch (items->type) {
>>> case RTE_FLOW_ITEM_TYPE_PORT_ID:
>>> @@ -4018,6 +4005,19 @@ struct field_modify_info modify_tcp[] = {
>>> }
>>> item_flags |= last_item;
>>> }
>>> + if (((attr->ingress && !attr->transfer) ||
>>> + (attr->transfer && !(item_flags & MLX5_FLOW_ITEM_PORT_ID)))
>> &&
>>> + (priv->representor || priv->master)) {
>>
>> From the validations, I could figure out
>> - Either ingress (I) or egress (E) must be specified
>> - Transfer (T) can't be egress
> 0> - Port ID (P) is valid only if transfer (T) is specified.
>>
>> (!T and I) or (T and !P)
>> = (I - T) + (T - P)
>> = I - P
>>
>> So, this condition is equivalent to
>> if (attr->ingress && !!(item_flags & MLX5_FLOW_ITEM_PORT_ID) &&
>> (priv->representor || priv->master)) {
>> ...
>> }
>>
>> Right?
Right that was my typo.
Thanks,
Yongseok
>>
>
> You are right that we correnlty only support ingress rules for E-Switch, I
> want to keep it open if in future we
> will support also egress for E-Switch rules, but I guess we can update it
> when it will be relevant.
> Regarding the if you wrote there should be only one ! not 2 since this code
> is relevant only if the user
> didn't specified port_id.
>
> Am I right?
>
>> If agreed, please add comment properly.
>>
>>> + /* It was validated - we support unidirection flows only. */
>>> + assert(!attr->egress);
>>
>> This comment and assert are there to mention ingress and egress are
>> exclusive.
>> Is it still relevant? Did you also test the patch with enabling DEBUG?
>>
>
> I will remove this code.
>
>>> + /*
>>> + * Add matching on source vport index only
>>> + * for ingress rules in E-Switch configurations.
>>> + */
>>
>> Please make this comment appropriate as well.
>>
>
> This comment is correct, due to the second part of the if (E-Switch mode is
> enabled, never mind if
> it is E-Switch rule or Nic rule), but I will remove this comment and add it
> as part of the if updated comment.
>
>> Thanks,
>> Yongseok
>>
>>> + if (flow_dv_translate_item_port_id(dev, match_mask,
>>> + match_value, NULL))
>>> + return -rte_errno;
>>> + }
>>> assert(!flow_dv_check_valid_spec(matcher.mask.buf,
>>> dev_flow->dv.value.buf));
>>> dev_flow->layers = item_flags;
>>> --
>>> 1.8.3.1
