From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 27C71A0C47; Wed, 27 Oct 2021 16:46:01 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E1B8D40E0F; Wed, 27 Oct 2021 16:46:00 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 10E2C407FF for ; Wed, 27 Oct 2021 16:45:59 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635345959; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4kvoDIyZ1WCFfzsKQVsteAUgk1QYDv8YDl7oJUwY2Ns=; b=ZH3OsRXtv5jmfuHhq0YYSVgFy/kfHhFEgDAiSgjTbMoxMnixRWII8Pl0sc6nKCMQe2NuU0 +Li9rCIh5gBh5dHWJytKkjX0s7NbtxlnwyU+OUB2lj8GZ5V/9x6I12ij22lru+wh3JPIiF rwVIgYyn1FinrPeQtVIsFdD02RTEa7I= Received: from mail-pf1-f200.google.com (mail-pf1-f200.google.com [209.85.210.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-126-5R30bqKENp6PsyadWo1I1g-1; Wed, 27 Oct 2021 10:45:57 -0400 X-MC-Unique: 5R30bqKENp6PsyadWo1I1g-1 Received: by mail-pf1-f200.google.com with SMTP id m199-20020a628cd0000000b0047c3444d941so704003pfd.11 for ; Wed, 27 Oct 2021 07:45:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4kvoDIyZ1WCFfzsKQVsteAUgk1QYDv8YDl7oJUwY2Ns=; b=TZ+uShuR8VLgS2Gn8B2QHwYwT6q5x/Wn1hY80Hd+FkIozeTNkhtWRanmHy34suLKjb 2hz0HZ7gCxBeFFE54I1oMgnnRf6UtnjuHJBGVHyIhsHyzbudfPNLrbRR7mfQ3xOY0p52 3QMTiZ9xlyB+lhhWTZde1MG3FpjgeDun7SfbSYoAysKW1qdl4+HVsCEKqorJ9iz5h0iw KvBxENR7+zlySTzaGkNu0zU2RhmjBPSj0RKuhgIvfYRYUGNcS+I1oINXWe1Q8Q0hNBtP Jwk89YK8WSs8qcmuJmoIwIADtGcm7yLKdNIfRHSs+MsYYXG1cDhWtij88y2/ogSNMYZF +W9w== X-Gm-Message-State: AOAM530z9xinb4K/AGk5GliTdS1rzcMQhNFgLjySqt3SQtXIXNwdOXKT QhTkHle5SvcaQgvD5zjk0RqMyKafI+JGmGwAiIlKHmIX+Du/fRwBabSZjNG6M3DPmmTEYoA2VXe 3txgBYh3w60E/AZBcAt4= X-Received: by 2002:a63:7c4f:: with SMTP id l15mr10049522pgn.273.1635345956774; Wed, 27 Oct 2021 07:45:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyXR7SH8ihJgSnNQtORAbYOZWPiHFHnZXX1ctW1IxKSBC9bMZVhdZBzTxh+9HqPKPFWDtkmZEbElu3nxgtInvM= X-Received: by 2002:a63:7c4f:: with SMTP id l15mr10049506pgn.273.1635345956551; Wed, 27 Oct 2021 07:45:56 -0700 (PDT) MIME-Version: 1.0 References: <20211018102045.255831-1-maxime.coquelin@redhat.com> <20211018102045.255831-2-maxime.coquelin@redhat.com> <3cf32ebd-47cd-05d0-c64f-67e9418839ba@oktetlabs.ru> <1424ee91-9407-593b-ab6b-817d88fc4ce3@oktetlabs.ru> <8ec46370-ffbb-1df2-a335-16c815db7166@redhat.com> In-Reply-To: <8ec46370-ffbb-1df2-a335-16c815db7166@redhat.com> From: Yuri Benditovich Date: Wed, 27 Oct 2021 17:45:44 +0300 Message-ID: To: Maxime Coquelin Cc: ": Yan Vugenfirer" , Andrew Rybchenko , dev@dpdk.org, chenbo.xia@intel.com, amorenoz@redhat.com, david.marchand@redhat.com, ferruh.yigit@intel.com, michaelba@nvidia.com, viacheslavo@nvidia.com, xiaoyun.li@intel.com, nelio.laranjeiro@6wind.com Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ybendito@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH v5 1/5] net/virtio: add initial RSS support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Oct 27, 2021 at 1:55 PM Maxime Coquelin wrote: > Hi, > > On 10/19/21 11:37, Andrew Rybchenko wrote: > > Hi Maxime, > > > > On 10/19/21 12:22 PM, Maxime Coquelin wrote: > >> Hi Andrew, > >> > >> On 10/19/21 09:30, Andrew Rybchenko wrote: > >>> On 10/18/21 1:20 PM, Maxime Coquelin wrote: > >>>> Provide the capability to update the hash key, hash types > >>>> and RETA table on the fly (without needing to stop/start > >>>> the device). However, the key length and the number of RETA > >>>> entries are fixed to 40B and 128 entries respectively. This > >>>> is done in order to simplify the design, but may be > >>>> revisited later as the Virtio spec provides this > >>>> flexibility. > >>>> > >>>> Note that only VIRTIO_NET_F_RSS support is implemented, > >>>> VIRTIO_NET_F_HASH_REPORT, which would enable reporting the > >>>> packet RSS hash calculated by the device into mbuf.rss, is > >>>> not yet supported. > >>>> > >>>> Regarding the default RSS configuration, it has been > >>>> chosen to use the default Intel ixgbe key as default key, > >>>> and default RETA is a simple modulo between the hash and > >>>> the number of Rx queues. > >>>> > >>>> Signed-off-by: Maxime Coquelin > > > > [snip] > > > >>>> + rss.unclassified_queue = 0; > >>>> + memcpy(rss.indirection_table, hw->rss_reta, > >>>> VIRTIO_NET_RSS_RETA_SIZE * sizeof(uint16_t)); > >>>> + rss.max_tx_vq = nb_queues; > >>> > >>> Is it guaranteed that driver is configured with equal number > >>> of Rx and Tx queues? Or is it not a problem otherwise? > >> > >> Virtio networking devices works with queue pairs. > > > > But it seems to me that I still can configure just 1 Rx queue > > and many Tx queues. Basically just non equal. > > The line is suspicious since I'd expect nb_queues to be > > a number of Rx queues in the function, but we set max_tx_vq > > count here. > > The Virtio spec says: > " > A driver sets max_tx_vq to inform a device how many transmit > virtqueues it may use (transmitq1. . .transmitq max_tx_vq). > " > > But looking at Qemu side, its value is interpreted as the number of > queue pairs setup by the driver, same as is handled virtqueue_pairs of > struct virtio_net_ctrl_mq when RSS is not supported. > > In this patch we are compatible with what is done in Qemu, and what is > done for multiqueue when RSS is not enabled. > > I don't get why the spec talks about transmit queues, Yan & Yuri, any > idea? > > Indeed QEMU reference code uses the max_tx_vq as a number of queue pairs the same way it uses a parameter of _MQ command. Mainly this is related to vhost start flow which assumes that there is some number of ready vq pairs. When the driver sets max_tx_vq it guarantees that it does not use more than max_tx_vq TX queues. Actual RX queues that will be used can be taken from the indirection table which contains indices of RX queues. > >>>> +static int > >>>> +virtio_dev_rss_init(struct rte_eth_dev *eth_dev) > >>>> +{ > >>>> + struct virtio_hw *hw = eth_dev->data->dev_private; > >>>> + uint16_t nb_rx_queues = eth_dev->data->nb_rx_queues; > >>>> + struct rte_eth_rss_conf *rss_conf; > >>>> + int ret, i; > >>>> + > >>>> + rss_conf = ð_dev->data->dev_conf.rx_adv_conf.rss_conf; > >>>> + > >>>> + ret = virtio_dev_get_rss_config(hw, &hw->rss_hash_types); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + if (rss_conf->rss_hf) { > >>>> + /* Ensure requested hash types are supported by the device > */ > >>>> + if (rss_conf->rss_hf & > >>>> ~virtio_to_ethdev_rss_offloads(hw->rss_hash_types)) > >>>> + return -EINVAL; > >>>> + > >>>> + hw->rss_hash_types = > >>>> ethdev_to_virtio_rss_offloads(rss_conf->rss_hf); > >>>> + } > >>>> + > >>>> + if (!hw->rss_key) { > >>>> + /* Setup default RSS key if not already setup by the user */ > >>>> + hw->rss_key = rte_malloc_socket("rss_key", > >>>> + VIRTIO_NET_RSS_KEY_SIZE, 0, > >>>> + eth_dev->device->numa_node); > >>>> + if (!hw->rss_key) { > >>>> + PMD_INIT_LOG(ERR, "Failed to allocate RSS key"); > >>>> + return -1; > >>>> + } > >>>> + > >>>> + if (rss_conf->rss_key && rss_conf->rss_key_len) { > >>>> + if (rss_conf->rss_key_len != VIRTIO_NET_RSS_KEY_SIZE) { > >>>> + PMD_INIT_LOG(ERR, "Driver only supports %u RSS key > >>>> length", > >>>> + VIRTIO_NET_RSS_KEY_SIZE); > >>>> + return -EINVAL; > >>>> + } > >>>> + memcpy(hw->rss_key, rss_conf->rss_key, > >>>> VIRTIO_NET_RSS_KEY_SIZE); > >>>> + } else { > >>>> + memcpy(hw->rss_key, rss_intel_key, > >>>> VIRTIO_NET_RSS_KEY_SIZE); > >>>> + } > >>> > >>> Above if should work in the case of reconfigure as well when > >>> array is already allocated. > >> > >> I'm not sure, because rte_eth_dev_rss_hash_update() API does not update > >> eth_dev->data->dev_conf.rx_adv_conf.rss_conf, so we may lose the RSS key > >> the user would have updated using this API. What do you think? > > > > I think, but I think it should be used correctly by the > > application. I.e. if application does reconfigure and > > provides same or new key, does not matter, it should be > > applied. Key could be a part of configure request and we > > should not ignore it in the case of reconfigure. > > > > Yes, I agree that it is a bit tricky, but still right > > logic. > > Ok. I'm applying your suggestion, which is to apply rss_conf whatever > rss_key was initialized or not before. > > >>>> + } > >>>> + > >>>> + if (!hw->rss_reta) { > >>>> + /* Setup default RSS reta if not already setup by the user */ > >>>> + hw->rss_reta = rte_malloc_socket("rss_reta", > >>>> + VIRTIO_NET_RSS_RETA_SIZE * sizeof(uint16_t), 0, > >>>> + eth_dev->device->numa_node); > >>>> + if (!hw->rss_reta) { > >>>> + PMD_INIT_LOG(ERR, "Failed to allocate RSS reta"); > >>>> + return -1; > >>>> + } > >>>> + for (i = 0; i < VIRTIO_NET_RSS_RETA_SIZE; i++) > >>>> + hw->rss_reta[i] = i % nb_rx_queues; > >>> > >>> How should it work in the case of reconfigure if a nubmer of Rx > >>> queue changes? > >> > >> Hmm, good catch! I did not think about this, and so did not test it. > >> > >> Not to lose user-provisionned reta in case of unrelated change, maybe I > >> should save the number of Rx queues when setting the reta (here and in > >> virtio_dev_rss_reta_update), and perform a re-initialization if it is > >> different? > > > > Yes, it is much harder than RSS key case since the data are not > > provided in dev_conf explicitly. So, we should guess here what > > to do. The simple solution is to reinitialize if number of RxQs > > change. I'd go this way. > > OK, this will be done in next revision. > > >>> Also I'm wondering how it works... > >>> virtio_dev_rss_init() is called from eth_virtio_dev_init() as > >>> well when a number of Rx queues is zero. I guess the reason is > >>> VIRTIO_PMD_DEFAULT_GUEST_FEATURES, but it looks very fragile. > >> > >> Yes, we only add VIRTIO_NET_F_RSS to the supported features in > >> virtio_dev_configure(), if Rx MQ mode is RSS. So virtio_dev_rss_init() > >> will never be called from eth_virtio_dev_init(). > >> > >> If I'm not mistaken, rte_eth_dev_configure() must be called it is stated > >> in the API documentation, so it will be negotiated if conditions are > >> met. > > > > As far as I know we can configure ethdev with zero number of > > RxQs, but non-zero number of Tx queues. E.g. traffic generator > > usecase. > > > > Division by zero is a guaranteed crash, so, I'd care > > about it explicitly in any case. Just do not try to rely > > on other checks faraway. > > Sure, adding a check on nb_rx_queues before doing any RSS init. > > Thanks, > Maxime > > Andrew. > > > >