Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet Steering) for non-pmd ports.
On 6/13/24 12:15, Eli Britstein wrote:
>
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Monday, 10 June 2024 15:53
>> To: Roi Dayan ; d...@openvswitch.org
>> Cc: Eli Britstein ; Maor Dickman ;
>> i.maxim...@ovn.org
>> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet
>> Steering) for non-pmd ports.
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 6/9/24 12:16, Roi Dayan via dev wrote:
>>> From: Eli Britstein
>>>
>>> In the cited commit, XPS was introduced. It is NA for non-pmd ports.
>>> Upon port creation it is indeed disabled, but at port reconfigure, the
>>> condition of netdev_is_pmd() is missing.
>>> As a result, XPS is configured, and such messages are repeating in the log:
>>> DBG|Core 2: New TX queue ID 0 for port 'v1_r'.
>>> Fix it.
>>
>> Hi, Eli. Thanks for the patch!
>>
>> While it's maybe true that it was an original intention to not have XPS
>> engaged
>> for non-PMD ports (frankly, I don't remember), the behavior was changed
>> quickly after in commit:
>> e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling
>> code.") The logic was centralized in the reconfiguration code and no port is
>> actually used until it went through datapath reconfiguration.
>>
>> And later we had AF_XDP ports introduced and even afxdp-nonpmd. For
>> these it is still important to have balanced use of Tx queues even if the
>> port is
>> not polled by PMD threads on Rx side.
>>
>> We also changed netdev_send() API to include 'concurrent_txq' flag to make
>> the netdev implementation know if it needs to lock the queue before using.
>> Default STATIC mode doesn't set this flag.
>> This also means that we can't actually supply Tx queue IDs to netdev_send()
>> out of range of the allocated queues, since netdev implementation will have
>> to lock every time otherwise. STATIC mode will use out-of-range queue IDs
>> with this change applied.
> Thanks for this explanation. Let me clarify the scenario:
> We add a veth port to netdev bridge (dpif-netdev). It doesn't have multiple
> queues, so it is 1.
> The "wanted_txqs" is 2 at the minimum - one for at least one PMD, and another
> for the main thread.
> The veth port is still configured as XPS, though there are no multiple TX
> queues.
> When I looked back to what condition to add, I saw the is_pmd in the cited
> commit.
> How about this then?
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 5196183ff..dac7de851 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6646,7 +6646,8 @@ reconfigure_datapath(struct dp_netdev *dp)
> if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
> netdev_n_txq(port->netdev) > 1) {
> port->txq_mode = TXQ_MODE_XPS_HASH;
> -} else if (netdev_n_txq(port->netdev) < wanted_txqs) {
> +} else if (netdev_n_txq(port->netdev) > 1 &&
> + netdev_n_txq(port->netdev) < wanted_txqs) {
> port->txq_mode = TXQ_MODE_XPS;
> } else {
> port->txq_mode = TXQ_MODE_STATIC;
>
This will still break non-pmd ports that support multi-queue, e.g.
afxdp-nonpmd.
>>
>> With that, I don't think we can accept this change. At the current state of
>> the
>> netdev API, dpif-netdev should never actually use Tx queue IDs out of the
>> allocated range and it must set 'concurrent_txq' flag whenever queues can be
>> shared, otherwise we'll get data races and crashes on out-of-range memory
>> accesses.
> Do you mean TXQ_MODE_STATIC is broken regardless of this change? I guess if
> so it
> is currently hidden as it is never used.
It's not broken, but it must not be used (and it is not being used) if number of
available queues is less than number of threads. If you have a device with 8
queues
and only 4 threads in OVS, TXQ_MODE_STATIC will be used and will work fine.
Why the debug log message is a problem?
>>
>> We should technically remove all the 'qid % n_txq' stuff from all the netdev
>> implementations and replace them with ovs_assert() on the API level. We
>> had a few patches for that in the past, but they didn't get proper attention
>> and went stale.
> Maybe, but it's not related to this commit.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> Fixes: 324c8374852a ("dpif-netdev: XPS (Transmit Packet Steering)
>>> implementation."
Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet Steering) for non-pmd ports.
>-Original Message-
>From: Ilya Maximets
>Sent: Monday, 10 June 2024 15:53
>To: Roi Dayan ; d...@openvswitch.org
>Cc: Eli Britstein ; Maor Dickman ;
>i.maxim...@ovn.org
>Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet
>Steering) for non-pmd ports.
>
>External email: Use caution opening links or attachments
>
>
>On 6/9/24 12:16, Roi Dayan via dev wrote:
>> From: Eli Britstein
>>
>> In the cited commit, XPS was introduced. It is NA for non-pmd ports.
>> Upon port creation it is indeed disabled, but at port reconfigure, the
>> condition of netdev_is_pmd() is missing.
>> As a result, XPS is configured, and such messages are repeating in the log:
>> DBG|Core 2: New TX queue ID 0 for port 'v1_r'.
>> Fix it.
>
>Hi, Eli. Thanks for the patch!
>
>While it's maybe true that it was an original intention to not have XPS engaged
>for non-PMD ports (frankly, I don't remember), the behavior was changed
>quickly after in commit:
> e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling
>code.") The logic was centralized in the reconfiguration code and no port is
>actually used until it went through datapath reconfiguration.
>
>And later we had AF_XDP ports introduced and even afxdp-nonpmd. For
>these it is still important to have balanced use of Tx queues even if the port
>is
>not polled by PMD threads on Rx side.
>
>We also changed netdev_send() API to include 'concurrent_txq' flag to make
>the netdev implementation know if it needs to lock the queue before using.
>Default STATIC mode doesn't set this flag.
>This also means that we can't actually supply Tx queue IDs to netdev_send()
>out of range of the allocated queues, since netdev implementation will have
>to lock every time otherwise. STATIC mode will use out-of-range queue IDs
>with this change applied.
Thanks for this explanation. Let me clarify the scenario:
We add a veth port to netdev bridge (dpif-netdev). It doesn't have multiple
queues, so it is 1.
The "wanted_txqs" is 2 at the minimum - one for at least one PMD, and another
for the main thread.
The veth port is still configured as XPS, though there are no multiple TX
queues.
When I looked back to what condition to add, I saw the is_pmd in the cited
commit.
How about this then?
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5196183ff..dac7de851 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6646,7 +6646,8 @@ reconfigure_datapath(struct dp_netdev *dp)
if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
netdev_n_txq(port->netdev) > 1) {
port->txq_mode = TXQ_MODE_XPS_HASH;
-} else if (netdev_n_txq(port->netdev) < wanted_txqs) {
+} else if (netdev_n_txq(port->netdev) > 1 &&
+ netdev_n_txq(port->netdev) < wanted_txqs) {
port->txq_mode = TXQ_MODE_XPS;
} else {
port->txq_mode = TXQ_MODE_STATIC;
>
>With that, I don't think we can accept this change. At the current state of
>the
>netdev API, dpif-netdev should never actually use Tx queue IDs out of the
>allocated range and it must set 'concurrent_txq' flag whenever queues can be
>shared, otherwise we'll get data races and crashes on out-of-range memory
>accesses.
Do you mean TXQ_MODE_STATIC is broken regardless of this change? I guess if so
it is currently hidden as it is never used.
>
>We should technically remove all the 'qid % n_txq' stuff from all the netdev
>implementations and replace them with ovs_assert() on the API level. We
>had a few patches for that in the past, but they didn't get proper attention
>and went stale.
Maybe, but it's not related to this commit.
>
>Best regards, Ilya Maximets.
>
>>
>> Fixes: 324c8374852a ("dpif-netdev: XPS (Transmit Packet Steering)
>> implementation.")
>> Signed-off-by: Eli Britstein
>> Acked-by: Roi Dayan
>> ---
>> lib/dpif-netdev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>> c7f9e149025e..94e1204575ea 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -6804,7 +6804,7 @@ reconfigure_datapath(struct dp_netdev *dp)
>> if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
>> netdev_n_txq(port->netdev) > 1) {
>> port->txq_mode = TXQ_MODE_XPS_HASH;
>> -} else if (netdev_n_txq(port->netdev) < wanted_txqs) {
>> +} else if (netdev_n_txq(port->netdev) < wanted_txqs &&
>> + netdev_is_pmd(port->netdev)) {
>> port->txq_mode = TXQ_MODE_XPS;
>> } else {
>> port->txq_mode = TXQ_MODE_STATIC;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet Steering) for non-pmd ports.
On 6/9/24 12:16, Roi Dayan via dev wrote:
> From: Eli Britstein
>
> In the cited commit, XPS was introduced. It is NA for non-pmd ports.
> Upon port creation it is indeed disabled, but at port reconfigure, the
> condition of netdev_is_pmd() is missing.
> As a result, XPS is configured, and such messages are repeating in the log:
> DBG|Core 2: New TX queue ID 0 for port 'v1_r'.
> Fix it.
Hi, Eli. Thanks for the patch!
While it's maybe true that it was an original intention to not have XPS
engaged for non-PMD ports (frankly, I don't remember), the behavior was
changed quickly after in commit:
e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling code.")
The logic was centralized in the reconfiguration code and no port is
actually used until it went through datapath reconfiguration.
And later we had AF_XDP ports introduced and even afxdp-nonpmd. For these
it is still important to have balanced use of Tx queues even if the port
is not polled by PMD threads on Rx side.
We also changed netdev_send() API to include 'concurrent_txq' flag to
make the netdev implementation know if it needs to lock the queue before
using. Default STATIC mode doesn't set this flag.
This also means that we can't actually supply Tx queue IDs to netdev_send()
out of range of the allocated queues, since netdev implementation will
have to lock every time otherwise. STATIC mode will use out-of-range
queue IDs with this change applied.
With that, I don't think we can accept this change. At the current state
of the netdev API, dpif-netdev should never actually use Tx queue IDs
out of the allocated range and it must set 'concurrent_txq' flag whenever
queues can be shared, otherwise we'll get data races and crashes on
out-of-range memory accesses.
We should technically remove all the 'qid % n_txq' stuff from all the
netdev implementations and replace them with ovs_assert() on the API
level. We had a few patches for that in the past, but they didn't get
proper attention and went stale.
Best regards, Ilya Maximets.
>
> Fixes: 324c8374852a ("dpif-netdev: XPS (Transmit Packet Steering)
> implementation.")
> Signed-off-by: Eli Britstein
> Acked-by: Roi Dayan
> ---
> lib/dpif-netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c7f9e149025e..94e1204575ea 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6804,7 +6804,7 @@ reconfigure_datapath(struct dp_netdev *dp)
> if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
> netdev_n_txq(port->netdev) > 1) {
> port->txq_mode = TXQ_MODE_XPS_HASH;
> -} else if (netdev_n_txq(port->netdev) < wanted_txqs) {
> +} else if (netdev_n_txq(port->netdev) < wanted_txqs &&
> netdev_is_pmd(port->netdev)) {
> port->txq_mode = TXQ_MODE_XPS;
> } else {
> port->txq_mode = TXQ_MODE_STATIC;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet Steering) for non-pmd ports.
Bleep bloop. Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 97 characters long (recommended limit is 79)
#29 FILE: lib/dpif-netdev.c:6807:
} else if (netdev_n_txq(port->netdev) < wanted_txqs &&
netdev_is_pmd(port->netdev)) {
Lines checked: 36, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev: Disable XPS (Transmit Packet Steering) for non-pmd ports.
From: Eli Britstein
In the cited commit, XPS was introduced. It is NA for non-pmd ports.
Upon port creation it is indeed disabled, but at port reconfigure, the
condition of netdev_is_pmd() is missing.
As a result, XPS is configured, and such messages are repeating in the log:
DBG|Core 2: New TX queue ID 0 for port 'v1_r'.
Fix it.
Fixes: 324c8374852a ("dpif-netdev: XPS (Transmit Packet Steering)
implementation.")
Signed-off-by: Eli Britstein
Acked-by: Roi Dayan
---
lib/dpif-netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c7f9e149025e..94e1204575ea 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6804,7 +6804,7 @@ reconfigure_datapath(struct dp_netdev *dp)
if (port->txq_requested_mode == TXQ_REQ_MODE_HASH &&
netdev_n_txq(port->netdev) > 1) {
port->txq_mode = TXQ_MODE_XPS_HASH;
-} else if (netdev_n_txq(port->netdev) < wanted_txqs) {
+} else if (netdev_n_txq(port->netdev) < wanted_txqs &&
netdev_is_pmd(port->netdev)) {
port->txq_mode = TXQ_MODE_XPS;
} else {
port->txq_mode = TXQ_MODE_STATIC;
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
