[dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"

[Declan Doherty]

On 25/10/16 13:57, Bruce Richardson wrote:
> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
>> On 24/10/16 15:51, Jan Blunck wrote:
>>> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
>>> <declan.doherty at intel.com> wrote:
>>>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>>>
>>>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>>>
>>>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>>>
>>>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>>>
>>>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>>>
>>>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>>>
>>>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>>>> configuration
>>>>>>>>> can be changed.
>>>>>>>>>
>>>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>>>> pool,
>>>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>>>> freed mempool.
>>>>>>>>>
>>>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>>>> reconfiguration:
>>>>>>>>>
>>>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>>>> failed
>>>>>>>>>
>>>>>>>>> Cc: <stable at dpdk.org>
>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>>>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>         struct bond_rx_queue *bd_rx_q;
>>>>>>>>>         struct bond_tx_queue *bd_tx_q;
>>>>>>>>>
>>>>>>>>> -       uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>>>> -       uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>>>>         int errval;
>>>>>>>>>         uint16_t q_id;
>>>>>>>>>
>>>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>>         /* Setup Rx Queues */
>>>>>>>>> -       /* Use existing queues, if any */
>>>>>>>>> -       for (q_id = old_nb_rx_queues;
>>>>>>>>> -            q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>>>> +       for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>>                 bd_rx_q = (struct bond_rx_queue
>>>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>>>
>>>>>>>>>                 errval =
>>>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>>         /* Setup Tx Queues */
>>>>>>>>> -       /* Use existing queues, if any */
>>>>>>>>> -       for (q_id = old_nb_tx_queues;
>>>>>>>>> -            q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>>>> +       for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>>                 bd_tx_q = (struct bond_tx_queue
>>>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>>>
>>>>>>>>>                 errval =
>>>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> --
>>>>>>>>> 2.7.4
>>>>>>>>>
>>>>>>>>
>>>>>>>> NAK
>>>>>>>>
>>>>>>>> There are still some users of this code.  Let's give them a chance to
>>>>>>>> comment before removing it.
>>>>>>>
>>>>>>>
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>>>> enough
>>>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>>>> that
>>>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>>>> devices
>>>>>>> in runtime including OVS.
>>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>>>> concern
>>>>>> just with regards to potential impact to others?
>>>>>>
>>>>>> Thanks,
>>>>>> /Bruce
>>>>>
>>>>>
>>>>> This won't impact me directly.  The users are CCed (different thread)
>>>>> and I haven't seen any comment, so I no longer have any objection to
>>>>> reverting this change.
>>>>>
>>>>> Eric
>>>>>
>>>>
>>>> As there has been no further objections and this reinstates the original
>>>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>>>> release.
>>>>
>>>> Acked-by: Declan Doherty <declan.doherty at intel.com>
>>>
>>> Ok, I can revert the revert for us.
>>>
>>> Do I read this correctly that you are not interested in fixing this properly?!
>>>
>>> Thanks,
>>> Jan
>>>
>>
>> Jan, sorry I missed the replies from last week due to the way my mail client
>> was filtering the conversation. Let me have another look at this and I'll
>> come back to the list.
>>
>> Thanks
>> Declan
>
> While this patch has already been applied to dpdk-next-net tree, it
> appears that there is still some ongoing discussion about it. I'm
> therefore planning to pull it back out of the tree for rc2. If a
> subsequent consensus is reached we can see about including it in rc3.
>
> Declan, as maintainer, does this seem reasonable to you.
>
> Regards,
> /Bruce
>


Hey Bruce, that seems reasonable, I would like to discuss this further 
with Jan and Ilya.

Declan