DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	"John W. Linville" <linville@tuxdriver.com>,
	Xiaolong Ye <xiaolong.ye@intel.com>,
	Qi Zhang <qi.z.zhang@intel.com>,
	Igor Russkikh <igor.russkikh@aquantia.com>,
	"Pavel Belous" <pavel.belous@aquantia.com>,
	Allain Legacy <allain.legacy@windriver.com>,
	Matt Peters <matt.peters@windriver.com>,
	"Ravi Kumar" <ravi1.kumar@amd.com>,
	Rasesh Mody <rmody@marvell.com>,
	Shahed Shaikh <shshaikh@marvell.com>,
	Ajit Khaparde <ajit.khaparde@broadcom.com>,
	"Somnath Kotur" <somnath.kotur@broadcom.com>,
	Chas Williams <chas3@att.com>,
	"Rahul Lakkireddy" <rahul.lakkireddy@chelsio.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>,
	Sachin Saxena <sachin.saxena@nxp.com>,
	Wenzhuo Lu <wenzhuo.lu@intel.com>,
	Gagandeep Singh <g.singh@nxp.com>,
	John Daley <johndale@cisco.com>,
	Hyong Youb Kim <hyonkim@cisco.com>,
	Gaetan Rivet <gaetan.rivet@6wind.com>,
	Xiao Wang <xiao.w.wang@intel.com>,
	Ziyang Xuan <xuanziyang2@huawei.com>,
	Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>,
	Guoyang Zhou <zhouguoyang@huawei.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Jingjing Wu <jingjing.wu@intel.com>,
	Qiming Yang <qiming.yang@intel.com>,
	Rosen Xu <rosen.xu@intel.com>,
	Konstantin Ananyev <konstantin.ananyev@intel.com>,
	Shijith Thotton <sthotton@marvell.com>,
	Srisivasubramanian Srinivasan <srinivasan@marvell.com>,
	Matan Azrad <matan@mellanox.com>,
	Shahaf Shuler <shahafs@mellanox.com>,
	Yongseok Koh <yskoh@mellanox.com>,
	Viacheslav Ovsiienko <viacheslavo@mellanox.com>,
	"Zyta Szpak" <zr@semihalf.com>, Liron Himi <lironh@marvell.com>,
	Tomasz Duszynski <tdu@semihalf.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Rastislav Cernay <cernay@netcope.com>,
	Jan Remes <remes@netcope.com>,
	Alejandro Lucero <alejandro.lucero@netronome.com>,
	Jerin Jacob <jerinj@marvell.com>,
	Nithin Dabilpuram <ndabilpuram@marvell.com>,
	"Kiran Kumar K" <kirankumark@marvell.com>,
	Keith Wiles <keith.wiles@intel.com>,
	Maciej Czekaj <mczekaj@marvell.com>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	Tiwei Bie <tiwei.bie@intel.com>,
	Zhihong Wang <zhihong.wang@intel.com>,
	Yong Wang <yongwang@vmware.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status
Date: Mon, 16 Sep 2019 18:45:09 +0300
Message-ID: <4cce6033-0347-04f6-3655-f36fbf9d32e8@solarflare.com> (raw)
In-Reply-To: <1696df72-998b-d189-aa19-70a16766f3ab@intel.com>

On 9/16/19 4:22 PM, Ferruh Yigit wrote:
> On 9/13/2019 8:57 PM, Andrew Rybchenko wrote:
>> On 9/13/19 7:34 PM, Ferruh Yigit wrote:
>>> On 9/13/2019 5:05 PM, Andrew Rybchenko wrote:
>>>> On 9/13/19 6:39 PM, Ferruh Yigit wrote:
>>>>> On 9/9/2019 12:58 PM, Andrew Rybchenko wrote:
>>>>>> Enabling/disabling of promiscuous mode is not always successful and
>>>>>> it should be taken into account to be able to handle it properly.
>>>>>>
>>>>>> When correct return status is unclear from driver code, -EAGAIN is used.
>>>>>>
>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> <...>
>>>>>
>>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>>> index f85458c3cd..41612ce838 100644
>>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>>> @@ -1100,28 +1100,60 @@ tap_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
>>>>>>     	return 0;
>>>>>>     }
>>>>>>     
>>>>>> -static void
>>>>>> +static int
>>>>>>     tap_promisc_enable(struct rte_eth_dev *dev)
>>>>>>     {
>>>>>>     	struct pmd_internals *pmd = dev->data->dev_private;
>>>>>>     	struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>>>>>> +	int ret;
>>>>>>     
>>>>>> -	dev->data->promiscuous = 1;
>>>>>> -	tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>>>>> -	if (pmd->remote_if_index && !pmd->flow_isolate)
>>>>>> -		tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>>>>>> +	ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>>>>> +	if (ret != 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (pmd->remote_if_index && !pmd->flow_isolate) {
>>>>>> +		dev->data->promiscuous = 1;
>>>>> I think PMD shouldn't be setting this variable, it is already set by the API.
>>>>> I quickly checked if an internal function requires this but it looks like it has
>>>>> been set by mistake, I think we can remove this.
>>>> It is set after callback in the case of enable.
>>> I see, but do we need it enabled earlier?
>> Not sure that I understand the question, but tap_ioctl() does not use it.
>> So, it is safe to move just before tap_flow_implicit_create().
> I think 'dev->data->promiscuous' shouldn't be set be PMD dev_ops, and API
> already does it. Is there a specific reason in tap to set this in dev_ops? If
> not can we remove setting 'dev->data->promiscuous' from dev_ops?

The problem is the following: right now enable function sets
data->promiscuous after driver callback execution, some drivers
definitely use the variable internally and require it to be set before
some driver operations. That's why these drivers set it on entry.
However, it does not mean that no drivers rely on the fact that
data->promiscuous value is set to 1 after callback. Most likely these
drivers are buggy since it is false in the case of configuration restore
on startup, but anyway it is not that simply.

Yes, I would prefer to avoid setting data->promiscuous from driver
code in enable/disable callbacks, but it should be a separate patch
which change it.

Right now:

net/bnxt sets data->promiscuous=1 on configure if Rx mode is VMDQ_DCB

net/mlx4 sets data->promiscuous always on enable/disable entry

net/octeontx sets data->promiscuous before exit from callback and
I think it can be safely removed since API functions definitely do it
before or after callback.

net/softnic sets data->promiscuous=1 on driver register

net/tap sets data->promiscuous=1 in enable/disable hooks before
tap_flow_implicit_create()

net/nfb sets data->promiscuous in driver init

net/octeontx2 sets data->promiscuous in the middle of processing, but
I guess before it is really used


>>>>>> +		ret = tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>>>>>> +		if (ret != 0) {
>>>>>> +			/* Rollback promisc flag */
>>>>>> +			tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>>>>> +			/*
>>>>>> +			 * rte_eth_dev_promiscuous_enable() rollback
>>>>>> +			 * dev->data->promiscuous in the case of failure.
>>>>>> +			 */
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>>     }
>>>>>>     
>>>>>> -static void
>>>>>> +static int
>>>>>>     tap_promisc_disable(struct rte_eth_dev *dev)
>>>>>>     {
>>>>>>     	struct pmd_internals *pmd = dev->data->dev_private;
>>>>>>     	struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>>>>>> +	int ret;
>>>>>>     
>>>>>> -	dev->data->promiscuous = 0;
>>>>>> -	tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>>>>> -	if (pmd->remote_if_index && !pmd->flow_isolate)
>>>>>> -		tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>>>>>> +	ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>>>>>> +	if (ret != 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (pmd->remote_if_index && !pmd->flow_isolate) {
>>>>>> +		dev->data->promiscuous = 0;
>>>>> Ditto
>>>>>
>>>>>> +		ret = tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>>>>>> +		if (ret != 0) {
>>>>>> +			/* Rollback promisc flag */
>>>>>> +			tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>>>>>> +			/*
>>>>>> +			 * rte_eth_dev_promiscuous_disable() rollback
>>>>>> +			 * dev->data->promiscuous in the case of failure.
>>>>>> +			 */
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>>     }


  reply index

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05 16:10 [dpdk-dev] [PATCH 00/13] ethdev: change promiscuous mode functions " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko
2019-09-05 16:25   ` Gaëtan Rivet
2019-09-05 16:32     ` Andrew Rybchenko
2019-09-05 16:36   ` Stephen Hemminger
2019-09-05 16:38     ` Andrew Rybchenko
2019-09-06  9:24       ` Gaëtan Rivet
2019-09-06 10:08         ` Andrew Rybchenko
2019-09-05 16:40   ` Stephen Hemminger
2019-09-05 16:49     ` Andrew Rybchenko
2019-09-05 17:13       ` Stephen Hemminger
2019-09-05 16:10 ` [dpdk-dev] [PATCH 03/13] net/bonding: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 07/13] app/testpmd: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 08/13] app/eventdev: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 09/13] app/pdump: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 10/13] app/test: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 11/13] kni: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 12/13] test/bonding: " Andrew Rybchenko
2019-09-05 16:10 ` [dpdk-dev] [PATCH 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko
2019-09-09 11:58 ` [dpdk-dev] [PATCH v2 00/13] ethdev: change promiscuous mode functions to return status Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko
2019-09-13 15:35     ` Ferruh Yigit
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko
2019-09-09 12:48     ` Gaëtan Rivet
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 03/13] net/bonding: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko
2019-09-10  7:53     ` Matan Azrad
2019-09-13 21:05       ` Andrew Rybchenko
2019-09-11  8:46     ` Hyong Youb Kim (hyonkim)
2019-09-13 21:06       ` Andrew Rybchenko
2019-09-16  6:48         ` Hyong Youb Kim (hyonkim)
2019-09-13 15:34     ` Ferruh Yigit
2019-09-13 15:54       ` Andrew Rybchenko
2019-09-13 16:33         ` Ferruh Yigit
2019-09-13 15:39     ` Ferruh Yigit
2019-09-13 16:05       ` Andrew Rybchenko
2019-09-13 16:34         ` Ferruh Yigit
2019-09-13 19:57           ` Andrew Rybchenko
2019-09-16 13:22             ` Ferruh Yigit
2019-09-16 15:45               ` Andrew Rybchenko [this message]
2019-09-16 15:58                 ` Ferruh Yigit
2019-09-13 16:43     ` Ferruh Yigit
2019-09-13 20:33       ` Andrew Rybchenko
2019-09-13 16:56     ` Ferruh Yigit
2019-09-13 20:38       ` Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 07/13] app/testpmd: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 08/13] app/eventdev: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 09/13] app/pdump: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 10/13] app/test: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 11/13] kni: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 12/13] test/bonding: " Andrew Rybchenko
2019-09-09 11:58   ` [dpdk-dev] [PATCH v2 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko
2019-09-13 16:40     ` Ferruh Yigit
2019-09-13 18:30       ` Andrew Rybchenko
2019-09-13 16:37   ` [dpdk-dev] [PATCH v2 00/13] ethdev: change promiscuous mode functions to return status Ferruh Yigit
2019-09-14 11:37 ` [dpdk-dev] [PATCH v3 " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 01/13] ethdev: change promiscuous mode controllers to return errors Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 02/13] net/failsafe: check code of promiscuous mode switch Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 03/13] net/bonding: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 04/13] ethdev: change promiscuous callbacks to return status Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 05/13] ethdev: do nothing if promiscuous mode is applied again Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 06/13] app/pipeline: check code of promiscuous mode switch Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 07/13] app/testpmd: " Andrew Rybchenko
2019-09-16 13:18     ` Iremonger, Bernard
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 08/13] app/eventdev: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 09/13] app/pdump: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 10/13] app/test: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 11/13] kni: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 12/13] test/bonding: " Andrew Rybchenko
2019-09-14 11:37   ` [dpdk-dev] [PATCH v3 13/13] examples: take promiscuous mode switch result into account Andrew Rybchenko
2019-09-24  7:31   ` [dpdk-dev] [PATCH v3 00/13] ethdev: change promiscuous mode functions to return status Ferruh Yigit

Reply instructions:

You may reply publically 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=4cce6033-0347-04f6-3655-f36fbf9d32e8@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=alejandro.lucero@netronome.com \
    --cc=allain.legacy@windriver.com \
    --cc=beilei.xing@intel.com \
    --cc=cernay@netcope.com \
    --cc=chas3@att.com \
    --cc=cloud.wangxiaoyun@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=g.singh@nxp.com \
    --cc=gaetan.rivet@6wind.com \
    --cc=haiyangz@microsoft.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=hyonkim@cisco.com \
    --cc=igor.russkikh@aquantia.com \
    --cc=jerinj@marvell.com \
    --cc=jingjing.wu@intel.com \
    --cc=johndale@cisco.com \
    --cc=keith.wiles@intel.com \
    --cc=kirankumark@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=kys@microsoft.com \
    --cc=linville@tuxdriver.com \
    --cc=lironh@marvell.com \
    --cc=matan@mellanox.com \
    --cc=matt.peters@windriver.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=mczekaj@marvell.com \
    --cc=ndabilpuram@marvell.com \
    --cc=pavel.belous@aquantia.com \
    --cc=qi.z.zhang@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=ravi1.kumar@amd.com \
    --cc=remes@netcope.com \
    --cc=rmody@marvell.com \
    --cc=rosen.xu@intel.com \
    --cc=sachin.saxena@nxp.com \
    --cc=shahafs@mellanox.com \
    --cc=shshaikh@marvell.com \
    --cc=somnath.kotur@broadcom.com \
    --cc=srinivasan@marvell.com \
    --cc=sthemmin@microsoft.com \
    --cc=sthotton@marvell.com \
    --cc=tdu@semihalf.com \
    --cc=thomas@monjalon.net \
    --cc=tiwei.bie@intel.com \
    --cc=viacheslavo@mellanox.com \
    --cc=wenzhuo.lu@intel.com \
    --cc=xiao.w.wang@intel.com \
    --cc=xiaolong.ye@intel.com \
    --cc=xuanziyang2@huawei.com \
    --cc=yongwang@vmware.com \
    --cc=yskoh@mellanox.com \
    --cc=zhihong.wang@intel.com \
    --cc=zhouguoyang@huawei.com \
    --cc=zr@semihalf.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

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox