[02/13] net/failsafe: check code of promiscuous mode switch
Checks
Commit Message
From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
value was changed from void to int, so this patch modify usage
of these functions across net/failsafe according to new return type.
Try to keep promiscuous mode consistent across all active
devices in the case of failure.
Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
drivers/net/failsafe/failsafe_ether.c | 8 +++--
drivers/net/failsafe/failsafe_ops.c | 44 ++++++++++++++++++++++++---
2 files changed, 46 insertions(+), 6 deletions(-)
Comments
Hi,
On Thu, Sep 05, 2019 at 05:10:40PM +0100, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>
> rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
> value was changed from void to int, so this patch modify usage
> of these functions across net/failsafe according to new return type.
>
> Try to keep promiscuous mode consistent across all active
> devices in the case of failure.
>
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> drivers/net/failsafe/failsafe_ether.c | 8 +++--
> drivers/net/failsafe/failsafe_ops.c | 44 ++++++++++++++++++++++++---
> 2 files changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index 504c76edb0..bd38f1c1e4 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -126,9 +126,13 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,
> if (dev->data->promiscuous != edev->data->promiscuous) {
> DEBUG("Configuring promiscuous");
> if (dev->data->promiscuous)
> - rte_eth_promiscuous_enable(PORT_ID(sdev));
> + ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
> else
> - rte_eth_promiscuous_disable(PORT_ID(sdev));
> + ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
> + if (ret != 0) {
> + ERROR("Failed to apply promiscuous mode");
> + return ret;
> + }
> } else {
> DEBUG("promiscuous already set");
> }
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index cc14bc2bcc..cbf143fb3c 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -659,11 +659,29 @@ fs_promiscuous_enable(struct rte_eth_dev *dev)
> {
> struct sub_device *sdev;
> uint8_t i;
> + int ret = 0;
>
> fs_lock(dev, 0);
> - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> - rte_eth_promiscuous_enable(PORT_ID(sdev));
> + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> + ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
> + if (ret != 0) {
> + ERROR("Promiscuous mode enable failed for subdevice %d",
> + PORT_ID(sdev));
> + break;
> + }
> + }
> + if (ret != 0) {
> + /* Rollback in the case of failure */
> + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> + ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
> + if (ret != 0)
> + ERROR("Promiscuous mode disable failed for subdevice %d",
> + PORT_ID(sdev));
> + }
> + }
> fs_unlock(dev, 0);
> +
> + return;
This patch should be applied after the ethdev change to avoid breaking
the build, I think?
You should be able to change the ethdev API, leaving the fail-safe
internals ignore the return value, then apply this patch and fix it.
This way the patchset should not break the build mid-series.
Otherwise good implementation with the rollback.
On 9/5/19 7:25 PM, Gaëtan Rivet wrote:
> Hi,
>
> On Thu, Sep 05, 2019 at 05:10:40PM +0100, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>>
>> rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
>> value was changed from void to int, so this patch modify usage
>> of these functions across net/failsafe according to new return type.
>>
>> Try to keep promiscuous mode consistent across all active
>> devices in the case of failure.
>>
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> drivers/net/failsafe/failsafe_ether.c | 8 +++--
>> drivers/net/failsafe/failsafe_ops.c | 44 ++++++++++++++++++++++++---
>> 2 files changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
>> index 504c76edb0..bd38f1c1e4 100644
>> --- a/drivers/net/failsafe/failsafe_ether.c
>> +++ b/drivers/net/failsafe/failsafe_ether.c
>> @@ -126,9 +126,13 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,
>> if (dev->data->promiscuous != edev->data->promiscuous) {
>> DEBUG("Configuring promiscuous");
>> if (dev->data->promiscuous)
>> - rte_eth_promiscuous_enable(PORT_ID(sdev));
>> + ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
>> else
>> - rte_eth_promiscuous_disable(PORT_ID(sdev));
>> + ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
>> + if (ret != 0) {
>> + ERROR("Failed to apply promiscuous mode");
>> + return ret;
>> + }
>> } else {
>> DEBUG("promiscuous already set");
>> }
>> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
>> index cc14bc2bcc..cbf143fb3c 100644
>> --- a/drivers/net/failsafe/failsafe_ops.c
>> +++ b/drivers/net/failsafe/failsafe_ops.c
>> @@ -659,11 +659,29 @@ fs_promiscuous_enable(struct rte_eth_dev *dev)
>> {
>> struct sub_device *sdev;
>> uint8_t i;
>> + int ret = 0;
>>
>> fs_lock(dev, 0);
>> - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
>> - rte_eth_promiscuous_enable(PORT_ID(sdev));
>> + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
>> + ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
>> + if (ret != 0) {
>> + ERROR("Promiscuous mode enable failed for subdevice %d",
>> + PORT_ID(sdev));
>> + break;
>> + }
>> + }
>> + if (ret != 0) {
>> + /* Rollback in the case of failure */
>> + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
>> + ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
>> + if (ret != 0)
>> + ERROR("Promiscuous mode disable failed for subdevice %d",
>> + PORT_ID(sdev));
>> + }
>> + }
>> fs_unlock(dev, 0);
>> +
>> + return;
> This patch should be applied after the ethdev change to avoid breaking
> the build, I think?
As far as I tested it does not break the build. Sorry for confusing
return at the end of function without return value.
The function is still void and it is updated to forward ret when
callback has return value.
> You should be able to change the ethdev API, leaving the fail-safe
> internals ignore the return value, then apply this patch and fix it.
> This way the patchset should not break the build mid-series.
>
> Otherwise good implementation with the rollback.
Thanks.
On Thu, 5 Sep 2019 17:10:40 +0100
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> + if (ret != 0) {
> + ERROR("Failed to apply promiscuous mode");
> + return ret;
> + }
Just return the error as normal and let caller deal with it.
Additional logging is not necessary.
On 9/5/19 7:36 PM, Stephen Hemminger wrote:
> On Thu, 5 Sep 2019 17:10:40 +0100
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> + if (ret != 0) {
>> + ERROR("Failed to apply promiscuous mode");
>> + return ret;
>> + }
> Just return the error as normal and let caller deal with it.
> Additional logging is not necessary.
In this particular case we simply follow style around.
No strong opinion. Up to Gaetan.
On Thu, 5 Sep 2019 17:10:40 +0100
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>
> rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
> value was changed from void to int, so this patch modify usage
> of these functions across net/failsafe according to new return type.
>
> Try to keep promiscuous mode consistent across all active
> devices in the case of failure.
>
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
In reality, failsafe's promiscious mode is actually a lie.
The only common use case of failsafe is on Azure/Hyper-V and in a guest
setting promiscuous is not allowed. That is why there is no promiscuous
setting in netvsc PMD.
On 9/5/19 7:40 PM, Stephen Hemminger wrote:
> On Thu, 5 Sep 2019 17:10:40 +0100
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>>
>> rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
>> value was changed from void to int, so this patch modify usage
>> of these functions across net/failsafe according to new return type.
>>
>> Try to keep promiscuous mode consistent across all active
>> devices in the case of failure.
>>
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> In reality, failsafe's promiscious mode is actually a lie.
> The only common use case of failsafe is on Azure/Hyper-V and in a guest
> setting promiscuous is not allowed. That is why there is no promiscuous
> setting in netvsc PMD.
Confused
$ git grep promiscuous drivers/net/netvsc/
drivers/net/netvsc/hn_ethdev.c:hn_dev_promiscuous_enable(struct
rte_eth_dev *dev)
drivers/net/netvsc/hn_ethdev.c: return hn_vf_promiscuous_enable(dev);
drivers/net/netvsc/hn_ethdev.c:hn_dev_promiscuous_disable(struct
rte_eth_dev *dev)
drivers/net/netvsc/hn_ethdev.c: return hn_vf_promiscuous_disable(dev);
drivers/net/netvsc/hn_ethdev.c: .promiscuous_enable =
hn_dev_promiscuous_enable,
drivers/net/netvsc/hn_ethdev.c: .promiscuous_disable =
hn_dev_promiscuous_disable,
drivers/net/netvsc/hn_var.h:int hn_vf_promiscuous_enable(struct
rte_eth_dev *dev);
drivers/net/netvsc/hn_var.h:int hn_vf_promiscuous_disable(struct
rte_eth_dev *dev);
drivers/net/netvsc/hn_vf.c:int hn_vf_promiscuous_enable(struct
rte_eth_dev *dev)
drivers/net/netvsc/hn_vf.c: VF_ETHDEV_FUNC_RET_STATUS(dev,
rte_eth_promiscuous_enable);
drivers/net/netvsc/hn_vf.c:int hn_vf_promiscuous_disable(struct
rte_eth_dev *dev)
drivers/net/netvsc/hn_vf.c: VF_ETHDEV_FUNC_RET_STATUS(dev,
rte_eth_promiscuous_disable);
On Thu, 5 Sep 2019 19:49:09 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> On 9/5/19 7:40 PM, Stephen Hemminger wrote:
> > On Thu, 5 Sep 2019 17:10:40 +0100
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >
> >> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> >>
> >> rte_eth_promiscuous_enable()/rte_eth_promiscuous_disable() return
> >> value was changed from void to int, so this patch modify usage
> >> of these functions across net/failsafe according to new return type.
> >>
> >> Try to keep promiscuous mode consistent across all active
> >> devices in the case of failure.
> >>
> >> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > In reality, failsafe's promiscious mode is actually a lie.
> > The only common use case of failsafe is on Azure/Hyper-V and in a guest
> > setting promiscuous is not allowed. That is why there is no promiscuous
> > setting in netvsc PMD.
I forgot Hyper-V is not same as Azure.
It will always fail on Azure, but will work be allowed on Hyper-V (if enabled on host).
On Thu, Sep 05, 2019 at 07:38:23PM +0300, Andrew Rybchenko wrote:
> On 9/5/19 7:36 PM, Stephen Hemminger wrote:
> > On Thu, 5 Sep 2019 17:10:40 +0100
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >
> > > + if (ret != 0) {
> > > + ERROR("Failed to apply promiscuous mode");
> > > + return ret;
> > > + }
> > Just return the error as normal and let caller deal with it.
> > Additional logging is not necessary.
>
> In this particular case we simply follow style around.
> No strong opinion. Up to Gaetan.
>
Ok, actually I missed a few things.
You should leave the error message in and keep it consistent with the rest.
However, you should test ret against fs_err(sdev, ret), you can find
other uses of it elsewhere in the file. When rte_eth_promiscuous_enable
returns -ENODEV, it is not an error for failsafe (as an absent device is
expected at some point).
Stephen if you would prefer never to have messages, it should be changed
all throughout the driver instead. If prefer it I would accept
a patch about it. Just want to keep it consistent.
On 9/6/19 12:24 PM, Gaëtan Rivet wrote:
> On Thu, Sep 05, 2019 at 07:38:23PM +0300, Andrew Rybchenko wrote:
>> On 9/5/19 7:36 PM, Stephen Hemminger wrote:
>>> On Thu, 5 Sep 2019 17:10:40 +0100
>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>
>>>> + if (ret != 0) {
>>>> + ERROR("Failed to apply promiscuous mode");
>>>> + return ret;
>>>> + }
>>> Just return the error as normal and let caller deal with it.
>>> Additional logging is not necessary.
>> In this particular case we simply follow style around.
>> No strong opinion. Up to Gaetan.
>>
> Ok, actually I missed a few things.
> You should leave the error message in and keep it consistent with the rest.
>
> However, you should test ret against fs_err(sdev, ret), you can find
> other uses of it elsewhere in the file. When rte_eth_promiscuous_enable
> returns -ENODEV, it is not an error for failsafe (as an absent device is
> expected at some point).
I see. Will fix in the next version.
> Stephen if you would prefer never to have messages, it should be changed
> all throughout the driver instead. If prefer it I would accept
> a patch about it. Just want to keep it consistent.
>
@@ -126,9 +126,13 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,
if (dev->data->promiscuous != edev->data->promiscuous) {
DEBUG("Configuring promiscuous");
if (dev->data->promiscuous)
- rte_eth_promiscuous_enable(PORT_ID(sdev));
+ ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
else
- rte_eth_promiscuous_disable(PORT_ID(sdev));
+ ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
+ if (ret != 0) {
+ ERROR("Failed to apply promiscuous mode");
+ return ret;
+ }
} else {
DEBUG("promiscuous already set");
}
@@ -659,11 +659,29 @@ fs_promiscuous_enable(struct rte_eth_dev *dev)
{
struct sub_device *sdev;
uint8_t i;
+ int ret = 0;
fs_lock(dev, 0);
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
- rte_eth_promiscuous_enable(PORT_ID(sdev));
+ FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
+ if (ret != 0) {
+ ERROR("Promiscuous mode enable failed for subdevice %d",
+ PORT_ID(sdev));
+ break;
+ }
+ }
+ if (ret != 0) {
+ /* Rollback in the case of failure */
+ FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
+ if (ret != 0)
+ ERROR("Promiscuous mode disable failed for subdevice %d",
+ PORT_ID(sdev));
+ }
+ }
fs_unlock(dev, 0);
+
+ return;
}
static void
@@ -671,11 +689,29 @@ fs_promiscuous_disable(struct rte_eth_dev *dev)
{
struct sub_device *sdev;
uint8_t i;
+ int ret = 0;
fs_lock(dev, 0);
- FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
- rte_eth_promiscuous_disable(PORT_ID(sdev));
+ FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
+ if (ret != 0) {
+ ERROR("Promiscuous mode disable failed for subdevice %d",
+ PORT_ID(sdev));
+ break;
+ }
+ }
+ if (ret != 0) {
+ /* Rollback in the case of failure */
+ FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+ ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
+ if (ret != 0)
+ ERROR("Promiscuous mode enable failed for subdevice %d",
+ PORT_ID(sdev));
+ }
+ }
fs_unlock(dev, 0);
+
+ return;
}
static void