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.
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 <[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
