>-----Original Message-----
>From: Ilya Maximets <[email protected]>
>Sent: Monday, 10 June 2024 15:53
>To: Roi Dayan <[email protected]>; [email protected]
>Cc: Eli Britstein <[email protected]>; Maor Dickman <[email protected]>;
>[email protected]
>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 <[email protected]>
>>
>> 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 <[email protected]>
>> Acked-by: Roi Dayan <[email protected]>
>> ---
>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
