Ferruh, I've been working on a patch but was distracted by other stuff and therefore haven't tested it yet.
Stay tuned, Jan On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit <ferruh.yigit at intel.com> wrote: > On 10/25/2016 3:00 PM, Bruce Richardson wrote: >> On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote: >>> 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. >>> >> >> Done. Hopefully consensus on a correct solution for this driver can be >> reached soon. >> > > Is there an update for this patch? Is a consensus reached? > > Thanks, > ferruh >