[dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Matan Azrad]

Missed 4 spaces were added after break if line.

Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index eefa868..aa33960 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1093,7 +1093,7 @@ struct tap_flow_items {
 				goto exit_action_not_supported;
 			action = 1;
 			if (!queue ||
-			(queue->index > pmd->dev->data->nb_rx_queues - 1))
+			    (queue->index > pmd->dev->data->nb_rx_queues - 1))
 				goto exit_action_not_supported;
 			if (flow)
 				err = add_action_skbedit(flow, queue->index);
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/2] net/tap: allow RSS flow action

[Matan Azrad]

One of the main identified use cases for the tap PMD is to be used in
combination with the fail-safe PMD as a fallback for a physical device.

Fail-safe is very strict about making sure its current configuration is
properly applied to all slave devices, they get rejected otherwise in
order to maintain a consistent state.

The problem is that tap's RSS support is currently limited to the
default (non-Toeplitz) balancing performed by the kernel on all Rx
queues. While proper RSS support emulation in the tap PMD is a work in
progress, the lack of rte_flow counterpart prevents validation of the
above use case in the meantime.

Given that unlike most PMDs, tap is more about convenience than
performance, support for the RSS action can be temporarily faked with
a minimum amount of code and mostly correct behavior by treating it
like a QUEUE action. Traffic is directed to the first queue of the set.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index aa33960..28d793f 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1089,6 +1089,7 @@ struct tap_flow_items {
 			const struct rte_flow_action_queue *queue =
 				(const struct rte_flow_action_queue *)
 				actions->conf;
+
 			if (action)
 				goto exit_action_not_supported;
 			action = 1;
@@ -1097,6 +1098,20 @@ struct tap_flow_items {
 				goto exit_action_not_supported;
 			if (flow)
 				err = add_action_skbedit(flow, queue->index);
+		} else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
+			/* Fake RSS support. */
+			const struct rte_flow_action_rss *rss =
+				(const struct rte_flow_action_rss *)
+				actions->conf;
+
+			if (action)
+				goto exit_action_not_supported;
+			action = 1;
+			if (!rss || rss->num < 1 ||
+			    (rss->queue[0] > pmd->dev->data->nb_rx_queues - 1))
+				goto exit_action_not_supported;
+			if (flow)
+				err = add_action_skbedit(flow, rss->queue[0]);
 		} else {
 			goto exit_action_not_supported;
 		}
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 2/2] net/tap: allow RSS flow action

[Pascal Mazon]

Hi,

What you say stands to reason, I'm basically ok with your patch, except
for one remark.
The rest of the file has no line breaks anywhere, please remove the two
you added.

Best regards,
Pascal

On 03/10/2017 16:55, Matan Azrad wrote:
> One of the main identified use cases for the tap PMD is to be used in
> combination with the fail-safe PMD as a fallback for a physical device.
>
> Fail-safe is very strict about making sure its current configuration is
> properly applied to all slave devices, they get rejected otherwise in
> order to maintain a consistent state.
>
> The problem is that tap's RSS support is currently limited to the
> default (non-Toeplitz) balancing performed by the kernel on all Rx
> queues. While proper RSS support emulation in the tap PMD is a work in
> progress, the lack of rte_flow counterpart prevents validation of the
> above use case in the meantime.
>
> Given that unlike most PMDs, tap is more about convenience than
> performance, support for the RSS action can be temporarily faked with
> a minimum amount of code and mostly correct behavior by treating it
> like a QUEUE action. Traffic is directed to the first queue of the set.
>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/tap/tap_flow.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index aa33960..28d793f 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1089,6 +1089,7 @@ struct tap_flow_items {
>  			const struct rte_flow_action_queue *queue =
>  				(const struct rte_flow_action_queue *)
>  				actions->conf;
> +
>  			if (action)
>  				goto exit_action_not_supported;
>  			action = 1;
> @@ -1097,6 +1098,20 @@ struct tap_flow_items {
>  				goto exit_action_not_supported;
>  			if (flow)
>  				err = add_action_skbedit(flow, queue->index);
> +		} else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
> +			/* Fake RSS support. */
> +			const struct rte_flow_action_rss *rss =
> +				(const struct rte_flow_action_rss *)
> +				actions->conf;
> +
> +			if (action)
> +				goto exit_action_not_supported;
> +			action = 1;
> +			if (!rss || rss->num < 1 ||
> +			    (rss->queue[0] > pmd->dev->data->nb_rx_queues - 1))
> +				goto exit_action_not_supported;
> +			if (flow)
> +				err = add_action_skbedit(flow, rss->queue[0]);
>  		} else {
>  			goto exit_action_not_supported;
>  		}

Re: [dpdk-dev] [PATCH 2/2] net/tap: allow RSS flow action

[Pascal Mazon]

Hi,

I was a little too quick earlier. The line breaks are justified because
they follow the list of variable declarations.

So I ack the whole patch series.

Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

On 04/10/2017 10:19, Pascal Mazon wrote:
> Hi,
>
> What you say stands to reason, I'm basically ok with your patch, except
> for one remark.
> The rest of the file has no line breaks anywhere, please remove the two
> you added.
>
> Best regards,
> Pascal
>
> On 03/10/2017 16:55, Matan Azrad wrote:
>> One of the main identified use cases for the tap PMD is to be used in
>> combination with the fail-safe PMD as a fallback for a physical device.
>>
>> Fail-safe is very strict about making sure its current configuration is
>> properly applied to all slave devices, they get rejected otherwise in
>> order to maintain a consistent state.
>>
>> The problem is that tap's RSS support is currently limited to the
>> default (non-Toeplitz) balancing performed by the kernel on all Rx
>> queues. While proper RSS support emulation in the tap PMD is a work in
>> progress, the lack of rte_flow counterpart prevents validation of the
>> above use case in the meantime.
>>
>> Given that unlike most PMDs, tap is more about convenience than
>> performance, support for the RSS action can be temporarily faked with
>> a minimum amount of code and mostly correct behavior by treating it
>> like a QUEUE action. Traffic is directed to the first queue of the set.
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>> ---
>>  drivers/net/tap/tap_flow.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
>> index aa33960..28d793f 100644
>> --- a/drivers/net/tap/tap_flow.c
>> +++ b/drivers/net/tap/tap_flow.c
>> @@ -1089,6 +1089,7 @@ struct tap_flow_items {
>>  			const struct rte_flow_action_queue *queue =
>>  				(const struct rte_flow_action_queue *)
>>  				actions->conf;
>> +
>>  			if (action)
>>  				goto exit_action_not_supported;
>>  			action = 1;
>> @@ -1097,6 +1098,20 @@ struct tap_flow_items {
>>  				goto exit_action_not_supported;
>>  			if (flow)
>>  				err = add_action_skbedit(flow, queue->index);
>> +		} else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
>> +			/* Fake RSS support. */
>> +			const struct rte_flow_action_rss *rss =
>> +				(const struct rte_flow_action_rss *)
>> +				actions->conf;
>> +
>> +			if (action)
>> +				goto exit_action_not_supported;
>> +			action = 1;
>> +			if (!rss || rss->num < 1 ||
>> +			    (rss->queue[0] > pmd->dev->data->nb_rx_queues - 1))
>> +				goto exit_action_not_supported;
>> +			if (flow)
>> +				err = add_action_skbedit(flow, rss->queue[0]);
>>  		} else {
>>  			goto exit_action_not_supported;
>>  		}

Re: [dpdk-dev] [PATCH 2/2] net/tap: allow RSS flow action

[Ferruh Yigit]

<...>

>> On 03/10/2017 16:55, Matan Azrad wrote:
>>> One of the main identified use cases for the tap PMD is to be used in
>>> combination with the fail-safe PMD as a fallback for a physical device.
>>>
>>> Fail-safe is very strict about making sure its current configuration is
>>> properly applied to all slave devices, they get rejected otherwise in
>>> order to maintain a consistent state.
>>>
>>> The problem is that tap's RSS support is currently limited to the
>>> default (non-Toeplitz) balancing performed by the kernel on all Rx
>>> queues. While proper RSS support emulation in the tap PMD is a work in
>>> progress, the lack of rte_flow counterpart prevents validation of the
>>> above use case in the meantime.
>>>
>>> Given that unlike most PMDs, tap is more about convenience than
>>> performance, support for the RSS action can be temporarily faked with
>>> a minimum amount of code and mostly correct behavior by treating it
>>> like a QUEUE action. Traffic is directed to the first queue of the set.
>>>
>>> Signed-off-by: Matan Azrad <matan@mellanox.com>

> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Ferruh Yigit]

On 10/3/2017 3:55 PM, Matan Azrad wrote:
> Missed 4 spaces were added after break if line.
> 
> Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")

What do you think fixing this when that code has been touched next time?

> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/tap/tap_flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index eefa868..aa33960 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1093,7 +1093,7 @@ struct tap_flow_items {
>  				goto exit_action_not_supported;
>  			action = 1;
>  			if (!queue ||
> -			(queue->index > pmd->dev->data->nb_rx_queues - 1))
> +			    (queue->index > pmd->dev->data->nb_rx_queues - 1))
>  				goto exit_action_not_supported;
>  			if (flow)
>  				err = add_action_skbedit(flow, queue->index);
> 

Re: [dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Pascal Mazon]

Hi,

I'm surprised there's only one place in the file with indent problem,
but I'm ok with the patch otherwise.

Ferruh, I didn't get what you mean; Matan is modifying tap_flow.c in
patch 2/2, right?

Best regards,
Pascal

On 04/10/2017 00:07, Ferruh Yigit wrote:
> On 10/3/2017 3:55 PM, Matan Azrad wrote:
>> Missed 4 spaces were added after break if line.
>>
>> Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")
> What do you think fixing this when that code has been touched next time?
>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>> ---
>>  drivers/net/tap/tap_flow.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
>> index eefa868..aa33960 100644
>> --- a/drivers/net/tap/tap_flow.c
>> +++ b/drivers/net/tap/tap_flow.c
>> @@ -1093,7 +1093,7 @@ struct tap_flow_items {
>>  				goto exit_action_not_supported;
>>  			action = 1;
>>  			if (!queue ||
>> -			(queue->index > pmd->dev->data->nb_rx_queues - 1))
>> +			    (queue->index > pmd->dev->data->nb_rx_queues - 1))
>>  				goto exit_action_not_supported;
>>  			if (flow)
>>  				err = add_action_skbedit(flow, queue->index);
>>

Re: [dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Ferruh Yigit]

On 10/4/2017 9:15 AM, Pascal Mazon wrote:
> Hi,
> 
> I'm surprised there's only one place in the file with indent problem,
> but I'm ok with the patch otherwise.
> 
> Ferruh, I didn't get what you mean; Matan is modifying tap_flow.c in
> patch 2/2, right?

I wasn't sure about getting the patch that fixes only syntax in one place.
Good to have proper syntax but a commit for this looked like overkill to
me. So I was suggesting keep it as it is and fix syntax when that piece
of code updated later.

But you are the maintainer of the driver, if you have strong opinion to
get it, sure I can.

Thanks,
ferruh

> 
> Best regards,
> Pascal
> 
> On 04/10/2017 00:07, Ferruh Yigit wrote:
>> On 10/3/2017 3:55 PM, Matan Azrad wrote:
>>> Missed 4 spaces were added after break if line.
>>>
>>> Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")
>> What do you think fixing this when that code has been touched next time?
>>
>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>> ---
>>>  drivers/net/tap/tap_flow.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
>>> index eefa868..aa33960 100644
>>> --- a/drivers/net/tap/tap_flow.c
>>> +++ b/drivers/net/tap/tap_flow.c
>>> @@ -1093,7 +1093,7 @@ struct tap_flow_items {
>>>  				goto exit_action_not_supported;
>>>  			action = 1;
>>>  			if (!queue ||
>>> -			(queue->index > pmd->dev->data->nb_rx_queues - 1))
>>> +			    (queue->index > pmd->dev->data->nb_rx_queues - 1))
>>>  				goto exit_action_not_supported;
>>>  			if (flow)
>>>  				err = add_action_skbedit(flow, queue->index);
>>>
> 

Re: [dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Ferruh Yigit]

On 10/4/2017 6:26 PM, Ferruh Yigit wrote:
> On 10/4/2017 9:15 AM, Pascal Mazon wrote:
>> Hi,
>>
>> I'm surprised there's only one place in the file with indent problem,
>> but I'm ok with the patch otherwise.
>>
>> Ferruh, I didn't get what you mean; Matan is modifying tap_flow.c in
>> patch 2/2, right?
> 
> I wasn't sure about getting the patch that fixes only syntax in one place.
> Good to have proper syntax but a commit for this looked like overkill to
> me. So I was suggesting keep it as it is and fix syntax when that piece
> of code updated later.
> 
> But you are the maintainer of the driver, if you have strong opinion to
> get it, sure I can.

What has been fixed is still in next-net, so I can squash this.

<...>

>>>> Missed 4 spaces were added after break if line.
>>>>
>>>> Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")
>>> What do you think fixing this when that code has been touched next time?
>>>
>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>

Squashed into relevant commit in next-net, thanks.

Re: [dpdk-dev] [PATCH 1/2] net/tap: fix indentation in flow file

[Pascal Mazon]

Hi,

Sorry for the delay, I was on vacation.

Nevermind this indent patch for now.
Matan, could you integrate it (squashed) when you'll have a new patch
modifying tap_flow.c?

Thank you.

Regards,
Pascal

On 05/10/2017 23:33, Ferruh Yigit wrote:
> On 10/4/2017 6:26 PM, Ferruh Yigit wrote:
>> On 10/4/2017 9:15 AM, Pascal Mazon wrote:
>>> Hi,
>>>
>>> I'm surprised there's only one place in the file with indent problem,
>>> but I'm ok with the patch otherwise.
>>>
>>> Ferruh, I didn't get what you mean; Matan is modifying tap_flow.c in
>>> patch 2/2, right?
>> I wasn't sure about getting the patch that fixes only syntax in one place.
>> Good to have proper syntax but a commit for this looked like overkill to
>> me. So I was suggesting keep it as it is and fix syntax when that piece
>> of code updated later.
>>
>> But you are the maintainer of the driver, if you have strong opinion to
>> get it, sure I can.
> What has been fixed is still in next-net, so I can squash this.
>
> <...>
>
>>>>> Missed 4 spaces were added after break if line.
>>>>>
>>>>> Fixes: 7a6811d78ed6 ("net/tap: fix flow and port commands")
>>>> What do you think fixing this when that code has been touched next time?
>>>>
>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Squashed into relevant commit in next-net, thanks.

Re: [dpdk-dev] [PATCH 2/2] net/tap: allow RSS flow action

[Matan Azrad]

I sent it from root user by mistake, sorry!
I Sent it again with my user.

> -----Original Message-----
> From: root [mailto:root@pegasus12.mtr.labs.mlnx]
> Sent: Tuesday, October 3, 2017 5:52 PM
> To: Pascal Mazon <pascal.mazon@6wind.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>
> Subject: [PATCH 2/2] net/tap: allow RSS flow action
> 
> From: Matan Azrad <matan@mellanox.com>
> 
> One of the main identified use cases for the tap PMD is to be used in
> combination with the fail-safe PMD as a fallback for a physical device.
> 
> Fail-safe is very strict about making sure its current configuration is properly
> applied to all slave devices, they get rejected otherwise in order to maintain
> a consistent state.
> 
> The problem is that tap's RSS support is currently limited to the default (non-
> Toeplitz) balancing performed by the kernel on all Rx queues. While proper
> RSS support emulation in the tap PMD is a work in progress, the lack of
> rte_flow counterpart prevents validation of the above use case in the
> meantime.
> 
> Given that unlike most PMDs, tap is more about convenience than
> performance, support for the RSS action can be temporarily faked with a
> minimum amount of code and mostly correct behavior by treating it like a
> QUEUE action. Traffic is directed to the first queue of the set.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/tap/tap_flow.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c index
> aa33960..28d793f 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1089,6 +1089,7 @@ struct tap_flow_items {
>  			const struct rte_flow_action_queue *queue =
>  				(const struct rte_flow_action_queue *)
>  				actions->conf;
> +
>  			if (action)
>  				goto exit_action_not_supported;
>  			action = 1;
> @@ -1097,6 +1098,20 @@ struct tap_flow_items {
>  				goto exit_action_not_supported;
>  			if (flow)
>  				err = add_action_skbedit(flow, queue-
> >index);
> +		} else if (actions->type == RTE_FLOW_ACTION_TYPE_RSS) {
> +			/* Fake RSS support. */
> +			const struct rte_flow_action_rss *rss =
> +				(const struct rte_flow_action_rss *)
> +				actions->conf;
> +
> +			if (action)
> +				goto exit_action_not_supported;
> +			action = 1;
> +			if (!rss || rss->num < 1 ||
> +			    (rss->queue[0] > pmd->dev->data->nb_rx_queues
> - 1))
> +				goto exit_action_not_supported;
> +			if (flow)
> +				err = add_action_skbedit(flow, rss-
> >queue[0]);
>  		} else {
>  			goto exit_action_not_supported;
>  		}
> --
> 1.8.3.1