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

Declan Doherty declan.doherty at intel.com
Mon Oct 24 17:07:17 CEST 2016


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


More information about the dev mailing list