[2/2] net/qede: restore Tx queue setup

  Some applications do not explicitly restore Tx queues setup during
port re-configuration. This patch adds changes to check for
released Tx queues and restore the setup if application doesn't
explicitly does that.

Signed-off-by: Rasesh Mody <rmody@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/net/qede/qede_ethdev.h |  3 +++
 drivers/net/qede/qede_rxtx.c   | 25 ++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)
  

On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com> wrote:
>
> Some applications do not explicitly restore Tx queues setup during
> port re-configuration. This patch adds changes to check for
> released Tx queues and restore the setup if application doesn't
> explicitly does that.

+ethdev maintainers.

I think, Ideally, the fix should be in common code if we need to
support such applications.

This would avoid the following case
- The application works only on a specific PMD.
- every PMD duplicating such restoration code.


>
> Signed-off-by: Rasesh Mody <rmody@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  drivers/net/qede/qede_ethdev.h |  3 +++
>  drivers/net/qede/qede_rxtx.c   | 25 ++++++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
> index b988a73f2..2e8e5febc 100644
> --- a/drivers/net/qede/qede_ethdev.h
> +++ b/drivers/net/qede/qede_ethdev.h
> @@ -235,6 +235,9 @@ struct qede_dev {
>         bool enable_lro;
>         uint8_t num_rx_queues;
>         uint8_t num_tx_queues;
> +       uint16_t num_rx_desc;
> +       uint16_t num_tx_desc;
> +       const struct rte_eth_txconf *tx_conf;
>         SLIST_HEAD(vlan_list_head, qede_vlan_entry)vlan_list_head;
>         uint16_t configured_vlans;
>         bool accept_any_vlan;
> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> index b81788ca4..1b212a4fb 100644
> --- a/drivers/net/qede/qede_rxtx.c
> +++ b/drivers/net/qede/qede_rxtx.c
> @@ -151,6 +151,7 @@ qede_alloc_rx_queue_mem(struct rte_eth_dev *dev,
>         rxq->qdev = qdev;
>         rxq->mb_pool = mp;
>         rxq->nb_rx_desc = nb_desc;
> +       qdev->num_rx_desc = rxq->nb_rx_desc;
>         rxq->queue_id = queue_idx;
>         rxq->port_id = dev->data->port_id;
>
> @@ -405,6 +406,7 @@ qede_alloc_tx_queue_mem(struct rte_eth_dev *dev,
>         }
>
>         txq->nb_tx_desc = nb_desc;
> +       qdev->num_tx_desc = txq->nb_tx_desc;
>         txq->qdev = qdev;
>         txq->port_id = dev->data->port_id;
>
> @@ -443,6 +445,7 @@ qede_alloc_tx_queue_mem(struct rte_eth_dev *dev,
>
>         txq->nb_tx_avail = txq->nb_tx_desc;
>
> +       qdev->tx_conf = tx_conf;
>         txq->tx_free_thresh =
>             tx_conf->tx_free_thresh ? tx_conf->tx_free_thresh :
>             (txq->nb_tx_desc - QEDE_DEFAULT_TX_FREE_THRESH);
> @@ -593,7 +596,7 @@ qede_alloc_mem_sb(struct qede_dev *qdev, struct ecore_sb_info *sb_info,
>
>  int qede_alloc_fp_resc(struct qede_dev *qdev)
>  {
> -       struct ecore_dev *edev = &qdev->edev;
> +       struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
>         struct qede_fastpath *fp;
>         uint32_t num_sbs;
>         uint16_t sb_idx;
> @@ -1005,9 +1008,29 @@ static int qede_tx_queue_stop(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id)
>  int qede_start_queues(struct rte_eth_dev *eth_dev)
>  {
>         struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
> +       struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
> +       struct qede_tx_queue *txq;
> +       struct qede_fastpath *fp;
>         uint8_t id;
>         int rc = -1;
>
> +       /* Restore setup of Tx queues */
> +       for (id = 0; id < qdev->num_tx_queues; id++) {
> +               fp = &qdev->fp_array[id];
> +               txq = fp->txq;
> +
> +               if (!txq) {
> +                       rc = qede_tx_queue_setup(eth_dev, id, qdev->num_tx_desc,
> +                                                eth_dev->data->numa_node,
> +                                                qdev->tx_conf);
> +                       if (rc != ECORE_SUCCESS) {
> +                               DP_ERR(edev, "TX queue %u not setup rc=%d\n",
> +                                      id, rc);
> +                               return -1;
> +                       }
> +               }
> +       }
> +
>         for (id = 0; id < qdev->num_rx_queues; id++) {
>                 rc = qede_rx_queue_start(eth_dev, id);
>                 if (rc != ECORE_SUCCESS)
> --
> 2.18.0
>
  

On 5/5/2020 7:44 AM, Jerin Jacob wrote:
> On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com> wrote:
>>
>> Some applications do not explicitly restore Tx queues setup during
>> port re-configuration. This patch adds changes to check for
>> released Tx queues and restore the setup if application doesn't
>> explicitly does that.
> 
> +ethdev maintainers.
> 
> I think, Ideally, the fix should be in common code if we need to
> support such applications.

Is this a case application re-configures to increase the number of queues but
doesn't setup new queues?
If so this looks like application error and application should be fixed instead
of recover this in the ethdev.

> 
> This would avoid the following case
> - The application works only on a specific PMD.
> - every PMD duplicating such restoration code.
> 
> 
>>
>> Signed-off-by: Rasesh Mody <rmody@marvell.com>
>> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
>> ---
>>  drivers/net/qede/qede_ethdev.h |  3 +++
>>  drivers/net/qede/qede_rxtx.c   | 25 ++++++++++++++++++++++++-
>>  2 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
>> index b988a73f2..2e8e5febc 100644
>> --- a/drivers/net/qede/qede_ethdev.h
>> +++ b/drivers/net/qede/qede_ethdev.h
>> @@ -235,6 +235,9 @@ struct qede_dev {
>>         bool enable_lro;
>>         uint8_t num_rx_queues;
>>         uint8_t num_tx_queues;
>> +       uint16_t num_rx_desc;
>> +       uint16_t num_tx_desc;
>> +       const struct rte_eth_txconf *tx_conf;
>>         SLIST_HEAD(vlan_list_head, qede_vlan_entry)vlan_list_head;
>>         uint16_t configured_vlans;
>>         bool accept_any_vlan;
>> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
>> index b81788ca4..1b212a4fb 100644
>> --- a/drivers/net/qede/qede_rxtx.c
>> +++ b/drivers/net/qede/qede_rxtx.c
>> @@ -151,6 +151,7 @@ qede_alloc_rx_queue_mem(struct rte_eth_dev *dev,
>>         rxq->qdev = qdev;
>>         rxq->mb_pool = mp;
>>         rxq->nb_rx_desc = nb_desc;
>> +       qdev->num_rx_desc = rxq->nb_rx_desc;
>>         rxq->queue_id = queue_idx;
>>         rxq->port_id = dev->data->port_id;
>>
>> @@ -405,6 +406,7 @@ qede_alloc_tx_queue_mem(struct rte_eth_dev *dev,
>>         }
>>
>>         txq->nb_tx_desc = nb_desc;
>> +       qdev->num_tx_desc = txq->nb_tx_desc;
>>         txq->qdev = qdev;
>>         txq->port_id = dev->data->port_id;
>>
>> @@ -443,6 +445,7 @@ qede_alloc_tx_queue_mem(struct rte_eth_dev *dev,
>>
>>         txq->nb_tx_avail = txq->nb_tx_desc;
>>
>> +       qdev->tx_conf = tx_conf;
>>         txq->tx_free_thresh =
>>             tx_conf->tx_free_thresh ? tx_conf->tx_free_thresh :
>>             (txq->nb_tx_desc - QEDE_DEFAULT_TX_FREE_THRESH);
>> @@ -593,7 +596,7 @@ qede_alloc_mem_sb(struct qede_dev *qdev, struct ecore_sb_info *sb_info,
>>
>>  int qede_alloc_fp_resc(struct qede_dev *qdev)
>>  {
>> -       struct ecore_dev *edev = &qdev->edev;
>> +       struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
>>         struct qede_fastpath *fp;
>>         uint32_t num_sbs;
>>         uint16_t sb_idx;
>> @@ -1005,9 +1008,29 @@ static int qede_tx_queue_stop(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id)
>>  int qede_start_queues(struct rte_eth_dev *eth_dev)
>>  {
>>         struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
>> +       struct ecore_dev *edev = QEDE_INIT_EDEV(qdev);
>> +       struct qede_tx_queue *txq;
>> +       struct qede_fastpath *fp;
>>         uint8_t id;
>>         int rc = -1;
>>
>> +       /* Restore setup of Tx queues */
>> +       for (id = 0; id < qdev->num_tx_queues; id++) {
>> +               fp = &qdev->fp_array[id];
>> +               txq = fp->txq;
>> +
>> +               if (!txq) {
>> +                       rc = qede_tx_queue_setup(eth_dev, id, qdev->num_tx_desc,
>> +                                                eth_dev->data->numa_node,
>> +                                                qdev->tx_conf);
>> +                       if (rc != ECORE_SUCCESS) {
>> +                               DP_ERR(edev, "TX queue %u not setup rc=%d\n",
>> +                                      id, rc);
>> +                               return -1;
>> +                       }
>> +               }
>> +       }
>> +
>>         for (id = 0; id < qdev->num_rx_queues; id++) {
>>                 rc = qede_rx_queue_start(eth_dev, id);
>>                 if (rc != ECORE_SUCCESS)
>> --
>> 2.18.0
>>
  

05/05/2020 10:59, Ferruh Yigit:
> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
> > On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com> wrote:
> >>
> >> Some applications do not explicitly restore Tx queues setup during
> >> port re-configuration. This patch adds changes to check for
> >> released Tx queues and restore the setup if application doesn't
> >> explicitly does that.
> > 
> > +ethdev maintainers.
> > 
> > I think, Ideally, the fix should be in common code if we need to
> > support such applications.
> 
> Is this a case application re-configures to increase the number of queues but
> doesn't setup new queues?
> If so this looks like application error and application should be fixed instead
> of recover this in the ethdev.

+1
  

Hi,

>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Tuesday, May 05, 2020 2:15 AM
>
>05/05/2020 10:59, Ferruh Yigit:
>> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
>> > On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
>wrote:
>> >>
>> >> Some applications do not explicitly restore Tx queues setup during
>> >> port re-configuration. This patch adds changes to check for
>> >> released Tx queues and restore the setup if application doesn't
>> >> explicitly does that.
>> >
>> > +ethdev maintainers.
>> >
>> > I think, Ideally, the fix should be in common code if we need to
>> > support such applications.
>>
>> Is this a case application re-configures to increase the number of
>> queues but doesn't setup new queues?
>> If so this looks like application error and application should be
>> fixed instead of recover this in the ethdev.
>
>+1
>

This is a case of KNI application performing device re-configuration to change MTU. The application explicitly calls Rx queue setup, however doesn't call Tx queue setup. When MTU for KNI interface is changed it runs into a segfault when trying to start Tx queues.
Some other applications make use of rte_eth_dev_set_mtu() ethdev op, which looks to be cleaner approach.

Thanks!
-Rasesh
  

On Wed, May 6, 2020 at 8:13 AM Rasesh Mody <rmody@marvell.com> wrote:
>
> Hi,
>
> >From: Thomas Monjalon <thomas@monjalon.net>
> >Sent: Tuesday, May 05, 2020 2:15 AM
> >
> >05/05/2020 10:59, Ferruh Yigit:
> >> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
> >> > On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
> >wrote:
> >> >>
> >> >> Some applications do not explicitly restore Tx queues setup during
> >> >> port re-configuration. This patch adds changes to check for
> >> >> released Tx queues and restore the setup if application doesn't
> >> >> explicitly does that.
> >> >
> >> > +ethdev maintainers.
> >> >
> >> > I think, Ideally, the fix should be in common code if we need to
> >> > support such applications.
> >>
> >> Is this a case application re-configures to increase the number of
> >> queues but doesn't setup new queues?
> >> If so this looks like application error and application should be
> >> fixed instead of recover this in the ethdev.
> >
> >+1
> >
>
> This is a case of KNI application performing device re-configuration to change MTU. The application explicitly calls Rx queue setup, however doesn't call Tx queue setup. When MTU for KNI interface is changed it runs into a segfault when trying to start Tx queues.

Can we fix the KNI application then?

> Some other applications make use of rte_eth_dev_set_mtu() ethdev op, which looks to be cleaner approach.
>
> Thanks!
> -Rasesh
>
  

Hi Jerin,

>From: Jerin Jacob <jerinjacobk@gmail.com>
>Sent: Sunday, May 10, 2020 12:04 AM
>
>On Wed, May 6, 2020 at 8:13 AM Rasesh Mody <rmody@marvell.com> wrote:
>>
>> Hi,
>>
>> >From: Thomas Monjalon <thomas@monjalon.net>
>> >Sent: Tuesday, May 05, 2020 2:15 AM
>> >
>> >05/05/2020 10:59, Ferruh Yigit:
>> >> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
>> >> > On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
>> >wrote:
>> >> >>
>> >> >> Some applications do not explicitly restore Tx queues setup
>> >> >> during port re-configuration. This patch adds changes to check
>> >> >> for released Tx queues and restore the setup if application
>> >> >> doesn't explicitly does that.
>> >> >
>> >> > +ethdev maintainers.
>> >> >
>> >> > I think, Ideally, the fix should be in common code if we need to
>> >> > support such applications.
>> >>
>> >> Is this a case application re-configures to increase the number of
>> >> queues but doesn't setup new queues?
>> >> If so this looks like application error and application should be
>> >> fixed instead of recover this in the ethdev.
>> >
>> >+1
>> >
>>
>> This is a case of KNI application performing device re-configuration to
>change MTU. The application explicitly calls Rx queue setup, however doesn't
>call Tx queue setup. When MTU for KNI interface is changed it runs into a
>segfault when trying to start Tx queues.
>
>Can we fix the KNI application then?

Yes, I have submitted a v2 series with changes to KNI application. Please apply for -rc3.

Thanks!
-Rasesh
>
>> Some other applications make use of rte_eth_dev_set_mtu() ethdev op,
>which looks to be cleaner approach.
>>
>> Thanks!
>> -Rasesh
>>
  

On Thu, May 14, 2020 at 9:40 AM Rasesh Mody <rmody@marvell.com> wrote:
>

> >> >> > +ethdev maintainers.
> >> >> >
> >> >> > I think, Ideally, the fix should be in common code if we need to
> >> >> > support such applications.
> >> >>
> >> >> Is this a case application re-configures to increase the number of
> >> >> queues but doesn't setup new queues?
> >> >> If so this looks like application error and application should be
> >> >> fixed instead of recover this in the ethdev.
> >> >
> >> >+1
> >> >
> >>
> >> This is a case of KNI application performing device re-configuration to
> >change MTU. The application explicitly calls Rx queue setup, however doesn't
> >call Tx queue setup. When MTU for KNI interface is changed it runs into a
> >segfault when trying to start Tx queues.
> >
> >Can we fix the KNI application then?
>
> Yes, I have submitted a v2 series with changes to KNI application. Please apply for -rc3.

Since it has the KNI app change, I have delegated the series to
@Ferruh Yigit to take it over next-net instead of next-net-mrvl.
  

On 5/6/2020 3:43 AM, Rasesh Mody wrote:
> Hi,
> 
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Tuesday, May 05, 2020 2:15 AM
>>
>> 05/05/2020 10:59, Ferruh Yigit:
>>> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
>>>> On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
>> wrote:
>>>>>
>>>>> Some applications do not explicitly restore Tx queues setup during
>>>>> port re-configuration. This patch adds changes to check for
>>>>> released Tx queues and restore the setup if application doesn't
>>>>> explicitly does that.
>>>>
>>>> +ethdev maintainers.
>>>>
>>>> I think, Ideally, the fix should be in common code if we need to
>>>> support such applications.
>>>
>>> Is this a case application re-configures to increase the number of
>>> queues but doesn't setup new queues?
>>> If so this looks like application error and application should be
>>> fixed instead of recover this in the ethdev.
>>
>> +1
>>
> 
> This is a case of KNI application performing device re-configuration to change MTU. The application explicitly calls Rx queue setup, however doesn't call Tx queue setup. When MTU for KNI interface is changed it runs into a segfault when trying to start Tx queues.

Why it crash? Device re-configuration should not be changing the number of the
queues, it is always 1. What is missing/wrong without the Tx queue setup?

> Some other applications make use of rte_eth_dev_set_mtu() ethdev op, which looks to be cleaner approach.
> 

+1. I don't know history if there is a specific reason this way selected, but
after release I think we can try this.
  

>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Thursday, May 14, 2020 7:33 AM
>
>On 5/6/2020 3:43 AM, Rasesh Mody wrote:
>> Hi,
>>
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> Sent: Tuesday, May 05, 2020 2:15 AM
>>>
>>> 05/05/2020 10:59, Ferruh Yigit:
>>>> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
>>>>> On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
>>> wrote:
>>>>>>
>>>>>> Some applications do not explicitly restore Tx queues setup during
>>>>>> port re-configuration. This patch adds changes to check for
>>>>>> released Tx queues and restore the setup if application doesn't
>>>>>> explicitly does that.
>>>>>
>>>>> +ethdev maintainers.
>>>>>
>>>>> I think, Ideally, the fix should be in common code if we need to
>>>>> support such applications.
>>>>
>>>> Is this a case application re-configures to increase the number of
>>>> queues but doesn't setup new queues?
>>>> If so this looks like application error and application should be
>>>> fixed instead of recover this in the ethdev.
>>>
>>> +1
>>>
>>
>> This is a case of KNI application performing device re-configuration to
>change MTU. The application explicitly calls Rx queue setup, however doesn't
>call Tx queue setup. When MTU for KNI interface is changed it runs into a
>segfault when trying to start Tx queues.
>
>Why it crash? Device re-configuration should not be changing the number of
>the queues, it is always 1. What is missing/wrong without the Tx queue setup?
>

QEDE PMD deallocates all fastpath resources unconditionally, including Tx queue, when re-configuring the device. When reallocating resources PMD relies on application to explicitly setup the Rx/Tx queue. 
Deallocation of all the resources is only required if the Rx/Tx queue configuration changes. So when KNI MTU change performs device re-configuration it exposes a bug, where there are no Tx queue resources setup. Then while starting the Tx queue in HW from PMD, application crashes.
I'll submit a PMD fix for device re-configuration.

>> Some other applications make use of rte_eth_dev_set_mtu() ethdev op,
>which looks to be cleaner approach.
>>
>
>+1. I don't know history if there is a specific reason this way
>+selected, but
>after release I think we can try this.

Sure.
Thanks!
-Rasesh
  

On 5/14/2020 11:43 PM, Rasesh Mody wrote:
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, May 14, 2020 7:33 AM
>>
>> On 5/6/2020 3:43 AM, Rasesh Mody wrote:
>>> Hi,
>>>
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> Sent: Tuesday, May 05, 2020 2:15 AM
>>>>
>>>> 05/05/2020 10:59, Ferruh Yigit:
>>>>> On 5/5/2020 7:44 AM, Jerin Jacob wrote:
>>>>>> On Tue, May 5, 2020 at 8:39 AM Rasesh Mody <rmody@marvell.com>
>>>> wrote:
>>>>>>>
>>>>>>> Some applications do not explicitly restore Tx queues setup during
>>>>>>> port re-configuration. This patch adds changes to check for
>>>>>>> released Tx queues and restore the setup if application doesn't
>>>>>>> explicitly does that.
>>>>>>
>>>>>> +ethdev maintainers.
>>>>>>
>>>>>> I think, Ideally, the fix should be in common code if we need to
>>>>>> support such applications.
>>>>>
>>>>> Is this a case application re-configures to increase the number of
>>>>> queues but doesn't setup new queues?
>>>>> If so this looks like application error and application should be
>>>>> fixed instead of recover this in the ethdev.
>>>>
>>>> +1
>>>>
>>>
>>> This is a case of KNI application performing device re-configuration to
>> change MTU. The application explicitly calls Rx queue setup, however doesn't
>> call Tx queue setup. When MTU for KNI interface is changed it runs into a
>> segfault when trying to start Tx queues.
>>
>> Why it crash? Device re-configuration should not be changing the number of
>> the queues, it is always 1. What is missing/wrong without the Tx queue setup?
>>
> 
> QEDE PMD deallocates all fastpath resources unconditionally, including Tx queue, when re-configuring the device. When reallocating resources PMD relies on application to explicitly setup the Rx/Tx queue. 
> Deallocation of all the resources is only required if the Rx/Tx queue configuration changes. So when KNI MTU change performs device re-configuration it exposes a bug, where there are no Tx queue resources setup. Then while starting the Tx queue in HW from PMD, application crashes.
> I'll submit a PMD fix for device re-configuration.

Got it.

But if there is no change in queue number in the re-configure, application
doesn't have to call the Rx/Tx setup. In this case even you fix the KNI sample
app, any other user application can hit same issue.

> 
>>> Some other applications make use of rte_eth_dev_set_mtu() ethdev op,
>> which looks to be cleaner approach.
>>>
>>
>> +1. I don't know history if there is a specific reason this way
>> +selected, but
>> after release I think we can try this.
> 
> Sure.
> Thanks!
> -Rasesh
>