> > >
> > > From: "Charles (Chas) Williams" <ch...@att.com>
> > >
> > > .dev_configure() may be called again after RX queues have been setup.
> > > This has the effect of clearing whatever setting the RX queues made for
> > > rx_bulk_alloc_allowed or rx_vec_allowed. Only reset this configuration
> > > is there aren't any currently allocated queues.
> > >
> > > Fixes: 01fa1d6215fa ("ixgbe: unify Rx setup")
> > >
> > > Signed-off-by: Chas Williams <ch...@att.com>
> > > ---
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++++++--
> > > 1 file changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 37eb668..b39249a 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -2348,6 +2348,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
> > > struct ixgbe_adapter *adapter =
> > > (struct ixgbe_adapter *)dev->data->dev_private;
> > > int ret;
> > > + uint16_t i;
> > >
> > > PMD_INIT_FUNC_TRACE();
> > > /* multipe queue mode checking */
> > > @@ -2363,11 +2364,17 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
> > >
> > > /*
> > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
> > > - * allocation or vector Rx preconditions we will reset it.
> > > + * allocation or vector Rx preconditions we will reset it. We
> > > + * can only do this is there aren't any existing RX queues.
> > > */
> > > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > + if (dev->data->rx_queues[i])
> > > + goto out;
> > > + }
> > I don't see why this is needed.
> > It surely should be possible to reconfigure device with different
> > number of queues.
> > Konstantin
> >
> > Yes, you can add new queues but you shouldn't reset the bulk and vec
> > settings
> > that have already been chosen by the previously allocated queues.
> Why is that? Might be in new queue setup user will change settings?
>
> There is no requirement that the user allocates all the RX queues in the same
> way.
> Some could have a different numbers of descs which is one of the checks in
> check_rx_burst_bulk_alloc_preconditions()
Exactly. That's why after dev_configure() user has to call queue_setup() for
*all*
queues he plans to use.
>
>
>
> > If those queues
> > set rx_bulk_alloc_allowed to be false, then this is going to cause an issue
> > with queue
> > release later on.
>
> Could you be a bit more specific here:
> What you think will be broken in ixgbe_rx_queue_release() in that case?
>
> Sorry, I mispoke. It's this function, ixgbe_reset_rx_queue(),
>
> /*
> * By default, the Rx queue setup function allocates enough memory for
> * IXGBE_MAX_RING_DESC. The Rx Burst bulk allocation function
> requires
> * extra memory at the end of the descriptor ring to be zero'd out.
> */
> if (adapter->rx_bulk_alloc_allowed)
> /* zero out extra memory */
> len += RTE_PMD_IXGBE_RX_MAX_BURST;
>
> /*
> * Zero out HW ring memory. Zero out extra memory at the end of
> * the H/W ring so look-ahead logic in Rx Burst bulk alloc function
> * reads extra memory as zeros.
> */
> for (i = 0; i < len; i++) {
> rxq->rx_ring[i] = zeroed_desc;
> }
>
> So you potentially write past the rx_ring[] you allocated.
We always allocate rx_ring[] to maximum possible size plus space for fake
descriptors:
drivers/net/ixgbe/ixgbe_rxtx.h:
#define RX_RING_SZ ((IXGBE_MAX_RING_DESC + RTE_PMD_IXGBE_RX_MAX_BURST) * \
sizeof(union ixgbe_adv_rx_desc))
then at ixgbe_dev_rx_queue_setup(...):
/*
* Allocate RX ring hardware descriptors. A memzone large enough to
* handle the maximum ring size is allocated in order to allow for
* resizing in later calls to the queue setup function.
*/
rz = rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx,
RX_RING_SZ, IXGBE_ALIGN, socket_id);
...
rxq->rx_ring = (union ixgbe_adv_rx_desc *) rz->addr;
>
> You can't change rx_bulk_alloc_allowed once it has been set and you have
> allocated queues.
> In fact, you can't really let some later queue change this setting after the
> first queue decides
> what itshould be.
See above.
>
>
>
> > This breaks:
> >
> > rte_eth_dev_configure(..., 1, 1, ...);
> > rx_queue_setup(1)
> > [rx_queue_setup decides that it can't support rx_bulk_alloc_allowed]
> > ..
> >
> > Later, you want to add some more queues, you call
> >
> > eth_eth_dev_configure(..., 2, 2, ...);
>
> After you call dev_configure, you'll have to do queue_setup() for all your
> queues.
> dev_configure() can changes some global device settings, so each queue has to
> be
> reconfigured.
> In your example It should be:
> eth_eth_dev_configure(..., 2, 2, ...);
> rx_queue_setup(...,0, ...);
> rx_queue_setup(...,1, ...);
>
> Konstantin
>
>
> Nothing in the API says this must happen. If there where true, shouldn't
> rte_eth_dev_configure()
> automatically drop any existing queues?
One of the fists things that ixgbe_dev_rx_queue_setup() - calls
ixgbe_rx_queue_release().
In theory that probably could be moved to rte_eth_dev_configure(), but I think
current model is ok too.
>There is existing code that doesn't do this.
Then I think it is a bug in that code.
> Show me
> in the API where I am required to setup all my queues _again_ after calling
> rte_eth_dev_configure()
If you think the documentation doesn't explain that properly - feel free to
raise a bug agains doc
and/or provide a patch for it.
As I explained above - dev_configure() might change some global settings
(max_rx_pkt, flow-director settings, etc.) that would affect each queue in
that device.
So queue_setup() is necessary after dev_configure() to make sure it will
operate properly.
Konstantin
>
>
> > rx_queue_setup(2)
> > [rx_queue_setup hopefully makes the same choice as rxqid = 1?]
> > ...
> >
> > Is one supposed to release all queues before calling
> > rte_eth_dev_configure()? If
> > that is true, it seems like the change_mtu examples I see are possibly
> > wrong. As
> > suggested in kenrel_nic_interface.rst:
> >
> >
> > ret = rte_eth_dev_configure(port_id, 1, 1, &conf);
> > if (ret < 0) {
> > RTE_LOG(ERR, APP, "Fail to reconfigure port %d\n", port_id);
> > return ret;
> > }
> >
> > /* Restart specific port */
> >
> > ret = rte_eth_dev_start(port_id);
> > if (ret < 0) {
> > RTE_LOG(ERR, APP, "Fail to restart port %d\n", port_id);
> > return ret;
> > }
> >
> > This is will obviously reset the rx_bulk_alloc_allowed and not reallocated
> > the RX queues.
> >
> >
> > > adapter->rx_bulk_alloc_allowed = true;
> > > adapter->rx_vec_allowed = true;
> > >
> > > +out:
> > > return 0;
> > > }
> > >
> > > @@ -4959,6 +4966,7 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev)
> > > struct rte_eth_conf *conf = &dev->data->dev_conf;
> > > struct ixgbe_adapter *adapter =
> > > (struct ixgbe_adapter *)dev->data->dev_private;
> > > + uint16_t i;
> > >
> > > PMD_INIT_LOG(DEBUG, "Configured Virtual Function port id: %d",
> > > dev->data->port_id);
> > > @@ -4981,11 +4989,17 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev)
> > >
> > > /*
> > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
> > > - * allocation or vector Rx preconditions we will reset it.
> > > + * allocation or vector Rx preconditions we will reset it. We
> > > + * can only do this is there aren't any existing RX queues.
> > > */
> > > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > + if (dev->data->rx_queues[i])
> > > + goto out;
> > > + }
> > > adapter->rx_bulk_alloc_allowed = true;
> > > adapter->rx_vec_allowed = true;
> > >
> > > +out:
> > > return 0;
> > > }
> > >
> > > --
> > > 2.9.5
