[v3,2/2] ethdev: device configuration enhancement
Checks
Commit Message
The new configuration is stored during the process.
But the process may fail. We better rolling the
configuration back as the new one doesn't take effect.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++------------
1 file changed, 43 insertions(+), 16 deletions(-)
Comments
On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> The new configuration is stored during the process.
> But the process may fail. We better rolling the
> configuration back as the new one doesn't take effect.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
I would say that the order is wrong. We should fix this bug first and
the changeset should have appropriate Fixes tags.
I think this bug is older and should be fixed first.
Then the second bug should be fixed without this one present.
On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
>> The new configuration is stored during the process.
>> But the process may fail. We better rolling the
>> configuration back as the new one doesn't take effect.
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>
> I would say that the order is wrong. We should fix this bug first and
> the changeset should have appropriate Fixes tags.
> I think this bug is older and should be fixed first.
> Then the second bug should be fixed without this one present.
Logically suggested order make sense I agree, but both patches are fixing defect
and order won't help backporting them [1], so no strong opinion about order.
Overall this patch should be converted into fix defect with proper Fixes tag
independent from order.
Wenzhuo, what do you think? I would like to get this one for rc3!
[1]
This is older defect but I believe can't be backported cleanly into older stable
trees because of "PMD-tuned Tx/Rx parameters" patches in the middle. Downside
having this first prevents other patch to backported to closer stable trees.
Also having this patch first will require additional return value update in some
checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation fixes this
order is clearer.
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Saturday, November 10, 2018 5:10 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> enhancement
>
> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> > On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> >> The new configuration is stored during the process.
> >> But the process may fail. We better rolling the configuration back as
> >> the new one doesn't take effect.
> >>
> >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >
> > I would say that the order is wrong. We should fix this bug first and
> > the changeset should have appropriate Fixes tags.
> > I think this bug is older and should be fixed first.
> > Then the second bug should be fixed without this one present.
>
> Logically suggested order make sense I agree, but both patches are fixing
> defect and order won't help backporting them [1], so no strong opinion
> about order.
>
> Overall this patch should be converted into fix defect with proper Fixes tag
> independent from order.
>
> Wenzhuo, what do you think? I would like to get this one for rc3!
>
>
> [1]
> This is older defect but I believe can't be backported cleanly into older stable
> trees because of "PMD-tuned Tx/Rx parameters" patches in the middle.
> Downside having this first prevents other patch to backported to closer
> stable trees.
>
> Also having this patch first will require additional return value update in
> some checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation
> fixes this order is clearer.
Yes, to my opinion, these 2 are separate patches. Actually there's no order between them. I put them together only because we have had a mixed discussion.
I didn't put a fix prefix because it's hard to add a fix tag for it. We know it has the problem from the beginning, so after some changes this patch cannot be backported.
On 11/13/2018 12:46 AM, Lu, Wenzhuo wrote:
> Hi Ferruh,
>
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Saturday, November 10, 2018 5:10 AM
>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
>> enhancement
>>
>> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
>>> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
>>>> The new configuration is stored during the process.
>>>> But the process may fail. We better rolling the configuration back as
>>>> the new one doesn't take effect.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>
>>> I would say that the order is wrong. We should fix this bug first and
>>> the changeset should have appropriate Fixes tags.
>>> I think this bug is older and should be fixed first.
>>> Then the second bug should be fixed without this one present.
>>
>> Logically suggested order make sense I agree, but both patches are fixing
>> defect and order won't help backporting them [1], so no strong opinion
>> about order.
>>
>> Overall this patch should be converted into fix defect with proper Fixes tag
>> independent from order.
>>
>> Wenzhuo, what do you think? I would like to get this one for rc3!
>>
>>
>> [1]
>> This is older defect but I believe can't be backported cleanly into older stable
>> trees because of "PMD-tuned Tx/Rx parameters" patches in the middle.
>> Downside having this first prevents other patch to backported to closer
>> stable trees.
>>
>> Also having this patch first will require additional return value update in
>> some checks (nb_tx_q && nb_rx_q checks) in next patch, so for separation
>> fixes this order is clearer.
> Yes, to my opinion, these 2 are separate patches. Actually there's no order between them. I put them together only because we have had a mixed discussion.
Yes they are not depends each other. Thinking twice adding first patch will
leave the code in a state more open the defect fixed in second patch. But by
fixing defect first second fix can be applied without having that open.
I will send a new version of the set.
> I didn't put a fix prefix because it's hard to add a fix tag for it. We know it has the problem from the beginning, so after some changes this patch cannot be backported.
>
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, November 13, 2018 5:41 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> enhancement
>
> On 11/13/2018 12:46 AM, Lu, Wenzhuo wrote:
> > Hi Ferruh,
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Saturday, November 10, 2018 5:10 AM
> >> To: Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: device configuration
> >> enhancement
> >>
> >> On 11/8/2018 6:25 AM, Andrew Rybchenko wrote:
> >>> On 11/8/18 5:09 AM, Wenzhuo Lu wrote:
> >>>> The new configuration is stored during the process.
> >>>> But the process may fail. We better rolling the configuration back
> >>>> as the new one doesn't take effect.
> >>>>
> >>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>>
> >>> I would say that the order is wrong. We should fix this bug first
> >>> and the changeset should have appropriate Fixes tags.
> >>> I think this bug is older and should be fixed first.
> >>> Then the second bug should be fixed without this one present.
> >>
> >> Logically suggested order make sense I agree, but both patches are
> >> fixing defect and order won't help backporting them [1], so no strong
> >> opinion about order.
> >>
> >> Overall this patch should be converted into fix defect with proper
> >> Fixes tag independent from order.
> >>
> >> Wenzhuo, what do you think? I would like to get this one for rc3!
> >>
> >>
> >> [1]
> >> This is older defect but I believe can't be backported cleanly into
> >> older stable trees because of "PMD-tuned Tx/Rx parameters" patches in
> the middle.
> >> Downside having this first prevents other patch to backported to
> >> closer stable trees.
> >>
> >> Also having this patch first will require additional return value
> >> update in some checks (nb_tx_q && nb_rx_q checks) in next patch, so
> >> for separation fixes this order is clearer.
> > Yes, to my opinion, these 2 are separate patches. Actually there's no order
> between them. I put them together only because we have had a mixed
> discussion.
>
> Yes they are not depends each other. Thinking twice adding first patch will
> leave the code in a state more open the defect fixed in second patch. But by
> fixing defect first second fix can be applied without having that open.
>
> I will send a new version of the set.
Thanks a lot! Very appreciate for your help!
>
> > I didn't put a fix prefix because it's hard to add a fix tag for it. We know it
> has the problem from the beginning, so after some changes this patch
> cannot be backported.
> >
@@ -1092,8 +1092,10 @@ struct rte_eth_dev *
{
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ struct rte_eth_conf orig_conf;
struct rte_eth_conf local_conf = *dev_conf;
int diag;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1106,12 +1108,16 @@ struct rte_eth_dev *
RTE_ETHDEV_LOG(ERR,
"Port %u must be stopped to allow configuration\n",
port_id);
- return -EBUSY;
+ ret = -EBUSY;
+ goto fail;
}
/* Copy the dev_conf parameter into the dev structure,
* then get the info.
+ * But restore the original configure first, as rollback is needed
+ * when failure happens.
*/
+ memcpy(&orig_conf, &dev->data->dev_conf, sizeof(dev->data->dev_conf));
memcpy(&dev->data->dev_conf, &local_conf, sizeof(dev->data->dev_conf));
rte_eth_dev_info_get(port_id, &dev_info);
@@ -1134,14 +1140,16 @@ struct rte_eth_dev *
RTE_ETHDEV_LOG(ERR,
"Number of RX queues requested (%u) is greater than max supported(%d)\n",
nb_rx_q, RTE_MAX_QUEUES_PER_PORT);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
if (nb_tx_q > RTE_MAX_QUEUES_PER_PORT) {
RTE_ETHDEV_LOG(ERR,
"Number of TX queues requested (%u) is greater than max supported(%d)\n",
nb_tx_q, RTE_MAX_QUEUES_PER_PORT);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
/*
@@ -1152,13 +1160,15 @@ struct rte_eth_dev *
if (nb_rx_q > dev_info.max_rx_queues) {
RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_rx_queues=%u > %u\n",
port_id, nb_rx_q, dev_info.max_rx_queues);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
if (nb_tx_q > dev_info.max_tx_queues) {
RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u nb_tx_queues=%u > %u\n",
port_id, nb_tx_q, dev_info.max_tx_queues);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
/* Check that the device supports requested interrupts */
@@ -1166,13 +1176,15 @@ struct rte_eth_dev *
(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC))) {
RTE_ETHDEV_LOG(ERR, "Driver %s does not support lsc\n",
dev->device->driver->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
if ((dev_conf->intr_conf.rmv == 1) &&
(!(dev->data->dev_flags & RTE_ETH_DEV_INTR_RMV))) {
RTE_ETHDEV_LOG(ERR, "Driver %s does not support rmv\n",
dev->device->driver->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
/*
@@ -1185,13 +1197,15 @@ struct rte_eth_dev *
"Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n",
port_id, dev_conf->rxmode.max_rx_pkt_len,
dev_info.max_rx_pktlen);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
} else if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
RTE_ETHDEV_LOG(ERR,
"Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n",
port_id, dev_conf->rxmode.max_rx_pkt_len,
(unsigned)ETHER_MIN_LEN);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
} else {
if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN ||
@@ -1210,7 +1224,8 @@ struct rte_eth_dev *
port_id, local_conf.rxmode.offloads,
dev_info.rx_offload_capa,
__func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
local_conf.txmode.offloads) {
@@ -1220,7 +1235,8 @@ struct rte_eth_dev *
port_id, local_conf.txmode.offloads,
dev_info.tx_offload_capa,
__func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
/* Check that device supports requested rss hash functions. */
@@ -1231,7 +1247,8 @@ struct rte_eth_dev *
"Ethdev port_id=%u invalid rss_hf: 0x%"PRIx64", valid value: 0x%"PRIx64"\n",
port_id, dev_conf->rx_adv_conf.rss_conf.rss_hf,
dev_info.flow_type_rss_offloads);
- return -EINVAL;
+ ret = -EINVAL;
+ goto rollback;
}
/*
@@ -1242,7 +1259,8 @@ struct rte_eth_dev *
RTE_ETHDEV_LOG(ERR,
"Port%u rte_eth_dev_rx_queue_config = %d\n",
port_id, diag);
- return diag;
+ ret = diag;
+ goto rollback;
}
diag = rte_eth_dev_tx_queue_config(dev, nb_tx_q);
@@ -1251,7 +1269,8 @@ struct rte_eth_dev *
"Port%u rte_eth_dev_tx_queue_config = %d\n",
port_id, diag);
rte_eth_dev_rx_queue_config(dev, 0);
- return diag;
+ ret = diag;
+ goto rollback;
}
diag = (*dev->dev_ops->dev_configure)(dev);
@@ -1260,7 +1279,8 @@ struct rte_eth_dev *
port_id, diag);
rte_eth_dev_rx_queue_config(dev, 0);
rte_eth_dev_tx_queue_config(dev, 0);
- return eth_err(port_id, diag);
+ ret = eth_err(port_id, diag);
+ goto rollback;
}
/* Initialize Rx profiling if enabled at compilation time. */
@@ -1270,10 +1290,17 @@ struct rte_eth_dev *
port_id, diag);
rte_eth_dev_rx_queue_config(dev, 0);
rte_eth_dev_tx_queue_config(dev, 0);
- return eth_err(port_id, diag);
+ ret = eth_err(port_id, diag);
+ goto rollback;
}
return 0;
+
+rollback:
+ memcpy(&dev->data->dev_conf, &orig_conf, sizeof(dev->data->dev_conf));
+
+fail:
+ return ret;
}
void