DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
	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
Subject: Re: [dpdk-dev] [PATCH v5 1/5] net/virtio: add initial RSS support
Date: Tue, 19 Oct 2021 12:37:43 +0300	[thread overview]
Message-ID: <1424ee91-9407-593b-ab6b-817d88fc4ce3@oktetlabs.ru> (raw)
In-Reply-To: <e53974cd-0272-ba22-6149-a9395110c455@redhat.com>

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 <maxime.coquelin@redhat.com>

[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 = &eth_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.

  reply	other threads:[~2021-10-19  9:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18 10:20 [dpdk-dev] [PATCH v5 0/5] Virtio PMD RSS support & RSS fixes Maxime Coquelin
2021-10-18 10:20 ` [dpdk-dev] [PATCH v5 1/5] net/virtio: add initial RSS support Maxime Coquelin
2021-10-19  4:47   ` Xia, Chenbo
2021-10-19  7:31     ` Maxime Coquelin
2021-10-19  7:30   ` Andrew Rybchenko
2021-10-19  9:22     ` Maxime Coquelin
2021-10-19  9:37       ` Andrew Rybchenko [this message]
2021-10-27 10:55         ` Maxime Coquelin
2021-10-27 14:45           ` Yuri Benditovich
2021-10-27 19:59             ` Maxime Coquelin
2021-10-18 10:20 ` [dpdk-dev] [PATCH v5 2/5] app/testpmd: fix RSS key length Maxime Coquelin
2021-10-18 10:20 ` [dpdk-dev] [PATCH v5 3/5] app/testpmd: fix RSS type display Maxime Coquelin
2021-10-18 10:20 ` [dpdk-dev] [PATCH v5 4/5] net/mlx5: fix RSS RETA update Maxime Coquelin
2021-10-18 10:20 ` [dpdk-dev] [PATCH v5 5/5] app/testpmd: add missing flow types in port info Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1424ee91-9407-593b-ab6b-817d88fc4ce3@oktetlabs.ru \
    --to=andrew.rybchenko@oktetlabs.ru \
    --cc=amorenoz@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=michaelba@nvidia.com \
    --cc=nelio.laranjeiro@6wind.com \
    --cc=viacheslavo@nvidia.com \
    --cc=xiaoyun.li@intel.com \
    --cc=ybendito@redhat.com \
    --cc=yvugenfi@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).