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 91CBEA0C43; Tue, 19 Oct 2021 11:37:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 837D6410EA; Tue, 19 Oct 2021 11:37:46 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 28D7340DF5 for ; Tue, 19 Oct 2021 11:37:44 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 812617F508; Tue, 19 Oct 2021 12:37:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 812617F508 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1634636263; bh=BxcJa9lk/EH/zzdBOZCgjkIzDqFExDbe3JZr3KYVK9w=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=iCpCQ0Jj7eefopxeh35qCtMFALOGLmJpob0EDn9wY6CClvzSMOKECRxEBWrLUSJS+ ndfjoj6T2oQfxlhiaE8zuxkwvzRrEGrlpthuzwraNfHN+MTM/x6qG1puDsirTjLuF6 vGTfGIbvJXH9lnlCTYvViINK/WzyCXHO1fvHtYLw= To: Maxime Coquelin , 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 Cc: nelio.laranjeiro@6wind.com, yvugenfi@redhat.com, ybendito@redhat.com References: <20211018102045.255831-1-maxime.coquelin@redhat.com> <20211018102045.255831-2-maxime.coquelin@redhat.com> <3cf32ebd-47cd-05d0-c64f-67e9418839ba@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <1424ee91-9407-593b-ab6b-817d88fc4ce3@oktetlabs.ru> Date: Tue, 19 Oct 2021 12:37:43 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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. >>> +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. >>> +    } >>> + >>> +    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. >> 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. Andrew.