Re: [dpdk-dev] [PATCH v5 1/5] net/virtio: add initial RSS support

[Yuri Benditovich <ybendito@redhat.com>]

On Wed, Oct 27, 2021 at 1:55 PM Maxime Coquelin <maxime.coquelin@redhat.com>
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 <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.
>
> 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 = &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.
>
> 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.
> >
>
>