From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <jingjing.wu@intel.com>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
by dpdk.org (Postfix) with ESMTP id EEC78C768
for <dev@dpdk.org>; Fri, 26 Jun 2015 09:03:38 +0200 (CEST)
Received: from fmsmga002.fm.intel.com ([10.253.24.26])
by fmsmga101.fm.intel.com with ESMTP; 26 Jun 2015 00:03:37 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.13,682,1427785200"; d="scan'208";a="750789921"
Received: from pgsmsx108.gar.corp.intel.com ([10.221.44.103])
by fmsmga002.fm.intel.com with ESMTP; 26 Jun 2015 00:03:36 -0700
Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by
PGSMSX108.gar.corp.intel.com (10.221.44.103) with Microsoft SMTP Server (TLS)
id 14.3.224.2; Fri, 26 Jun 2015 15:03:27 +0800
Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.129]) by
SHSMSX151.ccr.corp.intel.com ([169.254.3.168]) with mapi id 14.03.0224.002;
Fri, 26 Jun 2015 15:03:26 +0800
From: "Wu, Jingjing" <jingjing.wu@intel.com>
To: "'nhorman@tuxdriver.com'" <nhorman@tuxdriver.com>
Thread-Topic: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
Thread-Index: AQHQo0YzdTWZ6+MizUa530+v3dW9rJ2+cpXQ
Date: Fri, 26 Jun 2015 07:03:25 +0000
Message-ID: <9BB6961774997848B5B42BEC655768F8C59FF9@SHSMSX104.ccr.corp.intel.com>
References: <1433492166-30758-1-git-send-email-jingjing.wu@intel.com>
<1433917473-21508-1-git-send-email-jingjing.wu@intel.com>
<1433917473-21508-2-git-send-email-jingjing.wu@intel.com>
In-Reply-To: <1433917473-21508-2-git-send-email-jingjing.wu@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.239.127.40]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename
rte_eth_vmdq_mirror_conf
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 26 Jun 2015 07:03:40 -0000
Hi, Neil
About this patch I have an ABI concern about it.=20
This patch just renamed a struct rte_eth_vmdq_mirror_conf to rte_eth_mirror=
_conf, the size and its elements don't change. As my understanding, it will=
not break the ABI. And I also tested it.=20
But when I use the script ./scripts/validate-abi.sh to check. A low severit=
y problem is reported in symbol "rte_eth_mirror_rule_set"
- Change: "Base type of 2nd parameter mirror_conf has been changed from st=
ruct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf."
- Effect: "Replacement of parameter base type may indicate a change in its=
semantic meaning."
So, I'm not sure whether this patch meet the ABI policy?
Additional, about the validate-abi.sh, does it mean we need to fix all the =
problems it reports? Or we can decide case by case. Can a Low Severity prob=
lem be acceptable?
Look forward to your reply.
Thanks
Jingjing
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, June 10, 2015 2:25 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing; Liu, Jijiang; Jiajia, SunX; Zhang, Helin
> Subject: [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
>=20
> rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move the
> maximum rule id check from ethdev level to driver
>=20
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> ---
> app/test-pmd/cmdline.c | 22 +++++++++++-----------
> drivers/net/ixgbe/ixgbe_ethdev.c | 11 +++++++----
> drivers/net/ixgbe/ixgbe_ethdev.h | 4 +++-
> lib/librte_ether/rte_ethdev.c | 18 ++----------------
> lib/librte_ether/rte_ethdev.h | 19 ++++++++++---------
> 5 files changed, 33 insertions(+), 41 deletions(-)
>=20
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> f01db2a..d693bde 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -6604,11 +6604,11 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result, {
> int ret,nb_item,i;
> struct cmd_set_mirror_mask_result *res =3D parsed_result;
> - struct rte_eth_vmdq_mirror_conf mr_conf;
> + struct rte_eth_mirror_conf mr_conf;
>=20
> - memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> + memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
>=20
> - unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS];
> + unsigned int vlan_list[ETH_MIRROR_MAX_VLANS];
>=20
> mr_conf.dst_pool =3D res->dstpool_id;
>=20
> @@ -6618,11 +6618,11 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
> } else if(!strcmp(res->what, "vlan-mirror")) {
> mr_conf.rule_type_mask =3D ETH_VMDQ_VLAN_MIRROR;
> nb_item =3D parse_item_list(res->value, "core",
> -
> ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1);
> + ETH_MIRROR_MAX_VLANS, vlan_list,
> 1);
> if (nb_item <=3D 0)
> return;
>=20
> - for(i=3D0; i < nb_item; i++) {
> + for (i =3D 0; i < nb_item; i++) {
> if (vlan_list[i] > ETHER_MAX_VLAN_ID) {
> printf("Invalid vlan_id: must be < 4096\n");
> return;
> @@ -6634,10 +6634,10 @@ cmd_set_mirror_mask_parsed(void
> *parsed_result,
> }
>=20
> if(!strcmp(res->on, "on"))
> - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> res->rule_id, 1);
> else
> - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> res->rule_id, 0);
> if(ret < 0)
> printf("mirror rule add error: (%s)\n", strerror(-ret)); @@ -
> 6711,9 +6711,9 @@ cmd_set_mirror_link_parsed(void *parsed_result, {
> int ret;
> struct cmd_set_mirror_link_result *res =3D parsed_result;
> - struct rte_eth_vmdq_mirror_conf mr_conf;
> + struct rte_eth_mirror_conf mr_conf;
>=20
> - memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf));
> + memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf));
> if(!strcmp(res->what, "uplink-mirror")) {
> mr_conf.rule_type_mask =3D ETH_VMDQ_UPLINK_MIRROR;
> }else if(!strcmp(res->what, "downlink-mirror")) @@ -6722,10
> +6722,10 @@ cmd_set_mirror_link_parsed(void *parsed_result,
> mr_conf.dst_pool =3D res->dstpool_id;
>=20
> if(!strcmp(res->on, "on"))
> - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> res->rule_id, 1);
> else
> - ret =3D rte_eth_mirror_rule_set(res->port_id,&mr_conf,
> + ret =3D rte_eth_mirror_rule_set(res->port_id, &mr_conf,
> res->rule_id, 0);
>=20
> /* check the return value and print it if is < 0 */ diff --git
> a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 0d9f9b2..9e767fa 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -209,7 +209,7 @@ static int ixgbe_set_pool_tx(struct rte_eth_dev
> *dev,uint16_t pool,uint8_t on); static int ixgbe_set_pool_vlan_filter(st=
ruct
> rte_eth_dev *dev, uint16_t vlan,
> uint64_t pool_mask,uint8_t vlan_on);
> static int ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> - struct rte_eth_vmdq_mirror_conf *mirror_conf,
> + struct rte_eth_mirror_conf *mirror_conf,
> uint8_t rule_id, uint8_t on);
> static int ixgbe_mirror_rule_reset(struct rte_eth_dev *dev,
> uint8_t rule_id);
> @@ -3388,7 +3388,7 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev
> *dev, uint16_t vlan,
>=20
> static int
> ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> - struct rte_eth_vmdq_mirror_conf *mirror_conf,
> + struct rte_eth_mirror_conf *mirror_conf,
> uint8_t rule_id, uint8_t on)
> {
> uint32_t mr_ctl,vlvf;
> @@ -3412,7 +3412,10 @@ ixgbe_mirror_rule_set(struct rte_eth_dev *dev,
> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>=20
> if (ixgbe_vmdq_mode_check(hw) < 0)
> - return (-ENOTSUP);
> + return -ENOTSUP;
> +
> + if (rule_id >=3D IXGBE_MAX_MIRROR_RULES)
> + return -EINVAL;
>=20
> /* Check if vlan mask is valid */
> if ((mirror_conf->rule_type_mask & ETH_VMDQ_VLAN_MIRROR)
> && (on)) { @@ -3526,7 +3529,7 @@ ixgbe_mirror_rule_reset(struct
> rte_eth_dev *dev, uint8_t rule_id)
> return (-ENOTSUP);
>=20
> memset(&mr_info->mr_conf[rule_id], 0,
> - sizeof(struct rte_eth_vmdq_mirror_conf));
> + sizeof(struct rte_eth_mirror_conf));
>=20
> /* clear PFVMCTL register */
> IXGBE_WRITE_REG(hw, IXGBE_MRCTL(rule_id), mr_ctl); diff --git
> a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 19237b8..755b674 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -177,8 +177,10 @@ struct ixgbe_uta_info {
> uint32_t uta_shadow[IXGBE_MAX_UTA];
> };
>=20
> +#define IXGBE_MAX_MIRROR_RULES 4 /* Maximum nb. of mirror rules. */
> +
> struct ixgbe_mirror_info {
> - struct rte_eth_vmdq_mirror_conf
> mr_conf[ETH_VMDQ_NUM_MIRROR_RULE];
> + struct rte_eth_mirror_conf mr_conf[IXGBE_MAX_MIRROR_RULES];
> /**< store PF mirror rules configuration*/ };
>=20
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.=
c
> index 024fe8b..43c7295 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -3034,7 +3034,7 @@ int rte_eth_set_vf_rate_limit(uint8_t port_id,
> uint16_t vf, uint16_t tx_rate,
>=20
> int
> rte_eth_mirror_rule_set(uint8_t port_id,
> - struct rte_eth_vmdq_mirror_conf *mirror_conf,
> + struct rte_eth_mirror_conf *mirror_conf,
> uint8_t rule_id, uint8_t on)
> {
> struct rte_eth_dev *dev =3D &rte_eth_devices[port_id]; @@ -3051,7
> +3051,7 @@ rte_eth_mirror_rule_set(uint8_t port_id,
>=20
> if (mirror_conf->dst_pool >=3D ETH_64_POOLS) {
> PMD_DEBUG_TRACE("Invalid dst pool, pool id must"
> - "be 0-%d\n",ETH_64_POOLS - 1);
> + "be 0-%d\n", ETH_64_POOLS - 1);
> return -EINVAL;
> }
>=20
> @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id,
> return -EINVAL;
> }
>=20
> - if(rule_id >=3D ETH_VMDQ_NUM_MIRROR_RULE)
> - {
> - PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> - ETH_VMDQ_NUM_MIRROR_RULE - 1);
> - return -EINVAL;
> - }
> -
> dev =3D &rte_eth_devices[port_id];
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_set, -
> ENOTSUP);
>=20
> @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id,
> uint8_t rule_id)
> return -ENODEV;
> }
>=20
> - if(rule_id >=3D ETH_VMDQ_NUM_MIRROR_RULE)
> - {
> - PMD_DEBUG_TRACE("Invalid rule_id, rule_id must be 0-
> %d\n",
> - ETH_VMDQ_NUM_MIRROR_RULE-1);
> - return -EINVAL;
> - }
> -
> dev =3D &rte_eth_devices[port_id];
> FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mirror_rule_reset, -
> ENOTSUP);
>=20
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.=
h
> index 16dbe00..ae22fea 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -467,8 +467,8 @@ struct rte_eth_rss_conf {
> #define ETH_VMDQ_ACCEPT_BROADCAST 0x0008 /**< accept broadcast
> packets. */
> #define ETH_VMDQ_ACCEPT_MULTICAST 0x0010 /**< multicast
> promiscuous. */
>=20
> -/* Definitions used for VMDQ mirror rules setting */
> -#define ETH_VMDQ_NUM_MIRROR_RULE 4 /**< Maximum nb. of mirror
> rules. . */
> +/** Maximum nb. of vlan per mirror rule */
> +#define ETH_MIRROR_MAX_VLANS 64
>=20
> #define ETH_VMDQ_POOL_MIRROR 0x0001 /**< Virtual Pool Mirroring. */
> #define ETH_VMDQ_UPLINK_MIRROR 0x0002 /**< Uplink Port Mirroring.
> */ @@ -480,18 +480,19 @@ struct rte_eth_rss_conf {
> */
> struct rte_eth_vlan_mirror {
> uint64_t vlan_mask; /**< mask for valid VLAN ID. */
> - uint16_t vlan_id[ETH_VMDQ_MAX_VLAN_FILTERS];
> - /** VLAN ID list for vlan mirror. */
> + /** VLAN ID list for vlan mirroring. */
> + uint16_t vlan_id[ETH_MIRROR_MAX_VLANS];
> };
>=20
> /**
> * A structure used to configure traffic mirror of an Ethernet port.
> */
> -struct rte_eth_vmdq_mirror_conf {
> +struct rte_eth_mirror_conf {
> uint8_t rule_type_mask; /**< Mirroring rule type mask we want to
> set */
> - uint8_t dst_pool; /**< Destination pool for this mirror rule. */
> + uint8_t dst_pool; /**< Destination pool for this mirror rule. */
> uint64_t pool_mask; /**< Bitmap of pool for pool mirroring */
> - struct rte_eth_vlan_mirror vlan; /**< VLAN ID setting for VLAN
> mirroring */
> + /** VLAN ID setting for VLAN mirroring. */
> + struct rte_eth_vlan_mirror vlan;
> };
>=20
> /**
> @@ -1211,7 +1212,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct
> rte_eth_dev *dev, /**< @internal Set VF TX rate */
>=20
> typedef int (*eth_mirror_rule_set_t)(struct rte_eth_dev *dev,
> - struct rte_eth_vmdq_mirror_conf
> *mirror_conf,
> + struct rte_eth_mirror_conf *mirror_conf,
> uint8_t rule_id,
> uint8_t on);
> /**< @internal Add a traffic mirroring rule on an Ethernet device */ @@ =
-
> 3168,7 +3169,7 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port, uint16_t
> vlan_id,
> * - (-EINVAL) if the mr_conf information is not correct.
> */
> int rte_eth_mirror_rule_set(uint8_t port_id,
> - struct rte_eth_vmdq_mirror_conf *mirror_conf,
> + struct rte_eth_mirror_conf *mirror_conf,
> uint8_t rule_id,
> uint8_t on);
>=20
> --
> 1.9.3