From: Declan Doherty <declan.doherty@intel.com>
To: Jan Blunck <jblunck@infradead.org>
Cc: Eric Kinzie <ehkinzie@gmail.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Ilya Maximets <i.maximets@samsung.com>,
dev@dpdk.org, Heetae Ahn <heetae82.ahn@samsung.com>,
Yuanhan Liu <yuanhan.liu@linux.intel.com>,
Bernard Iremonger <bernard.iremonger@intel.com>,
stable@dpdk.org, Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
Date: Mon, 24 Oct 2016 16:07:17 +0100 [thread overview]
Message-ID: <ac86be29-61b2-11b3-5140-6004fec95ce2@intel.com> (raw)
In-Reply-To: <CALe+Z01fKQGVMUMPC=-uFr3aeGGOXcvnRyeF7HB-n6pLU2V_MQ@mail.gmail.com>
On 24/10/16 15:51, Jan Blunck wrote:
> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> <declan.doherty@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@dpdk.org>
>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@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@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
next prev parent reply other threads:[~2016-10-24 15:14 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-07 12:28 [dpdk-stable] " Ilya Maximets
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
2016-09-16 5:03 ` Ilya Maximets
2016-10-06 14:32 ` Declan Doherty
2016-10-19 9:55 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2016-10-25 13:59 ` Bruce Richardson
2016-10-07 2:02 ` [dpdk-stable] " Eric Kinzie
2016-10-12 13:24 ` Ilya Maximets
2016-10-12 15:24 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2016-10-13 23:37 ` Eric Kinzie
2016-10-24 11:02 ` Declan Doherty
2016-10-24 14:51 ` Jan Blunck
2016-10-24 15:07 ` Declan Doherty [this message]
2016-10-25 12:57 ` Bruce Richardson
2016-10-25 13:48 ` Declan Doherty
2016-10-25 14:00 ` Bruce Richardson
2016-11-21 11:30 ` Ferruh Yigit
2016-11-21 11:39 ` Jan Blunck
2016-11-21 12:49 ` Ilya Maximets
2016-11-21 13:11 ` Ilya Maximets
2016-11-23 20:35 ` Jan Blunck
2016-10-18 12:28 ` Jan Blunck
2016-10-18 12:49 ` Ilya Maximets
2016-10-18 15:19 ` Jan Blunck
2016-10-19 9:47 ` Ilya Maximets
2016-10-24 14:54 ` Jan Blunck
2016-10-25 6:26 ` Ilya Maximets
2016-10-28 6:14 ` Ilya Maximets
2016-11-11 9:16 ` Ilya Maximets
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac86be29-61b2-11b3-5140-6004fec95ce2@intel.com \
--to=declan.doherty@intel.com \
--cc=bernard.iremonger@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ehkinzie@gmail.com \
--cc=heetae82.ahn@samsung.com \
--cc=i.maximets@samsung.com \
--cc=jblunck@infradead.org \
--cc=stable@dpdk.org \
--cc=thomas.monjalon@6wind.com \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).