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 64C9DA0547; Wed, 27 Oct 2021 12:55:55 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EEBF04068C; Wed, 27 Oct 2021 12:55:54 +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 58A9E4003F for ; Wed, 27 Oct 2021 12:55:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635332152; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tTE8zSDaPa3ApjClKpQVK1cL2UkCzcEVn4obBKfzcsk=; b=dht5FJHm8QB2zuN8xWL5hQArg1hnt6HckDo7KbffdM84iATA+jkDXrpL7si+abN1Rr7VDI GFlpNF5+q1Y2GU5HKfv/0NIxvLzjLS3L8QwaFERd6Qm5PLIe7PctKW3uZiLsMh43NLXPOu IetX4yz9UkFtOGGrglu6tiKBZ2AXCiA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-176-fQ-IQoAKOAqw4QKZPu3EiA-1; Wed, 27 Oct 2021 06:55:49 -0400 X-MC-Unique: fQ-IQoAKOAqw4QKZPu3EiA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0375D802B78; Wed, 27 Oct 2021 10:55:48 +0000 (UTC) Received: from [10.39.208.21] (unknown [10.39.208.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E59A95C232; Wed, 27 Oct 2021 10:55:41 +0000 (UTC) Message-ID: <8ec46370-ffbb-1df2-a335-16c815db7166@redhat.com> Date: Wed, 27 Oct 2021 12:55:40 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 To: yvugenfi@redhat.com, ybendito@redhat.com, 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 Cc: nelio.laranjeiro@6wind.com 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> From: Maxime Coquelin In-Reply-To: <1424ee91-9407-593b-ab6b-817d88fc4ce3@oktetlabs.ru> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed 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, 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? >>>> +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. >