From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 362E2C38C for ; Mon, 6 Jul 2015 03:27:35 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 05 Jul 2015 18:27:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,412,1432623600"; d="scan'208";a="741127705" Received: from kmsmsx153.gar.corp.intel.com ([172.21.73.88]) by fmsmga001.fm.intel.com with ESMTP; 05 Jul 2015 18:27:32 -0700 Received: from kmsmsx154.gar.corp.intel.com (172.21.73.14) by KMSMSX153.gar.corp.intel.com (172.21.73.88) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 6 Jul 2015 09:27:31 +0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by KMSMSX154.gar.corp.intel.com (172.21.73.14) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 6 Jul 2015 09:27:30 +0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.129]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.246]) with mapi id 14.03.0224.002; Mon, 6 Jul 2015 09:27:29 +0800 From: "Wu, Jingjing" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf Thread-Index: AQHQr95LZE+zlENrOE6EI/HnX++9YZ3NtWpg Date: Mon, 6 Jul 2015 01:27:28 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F8C5FE27@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> <9BB6961774997848B5B42BEC655768F8C59FF9@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <9BB6961774997848B5B42BEC655768F8C59FF9@SHSMSX104.ccr.corp.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" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jul 2015 01:27:36 -0000 Hi, Thomas Any suggestions about this patch? Do I need rework it by using macro RTE_NE= XT_ABI? Thanks a lot! Jingjing > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wu, Jingjing > Sent: Friday, June 26, 2015 3:03 PM > To: 'nhorman@tuxdriver.com' > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename > rte_eth_vmdq_mirror_conf >=20 > Hi, Neil >=20 > About this patch I have an ABI concern about it. > 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. > But when I use the script ./scripts/validate-abi.sh to check. A low sever= ity > problem is reported in symbol "rte_eth_mirror_rule_set" > - Change: "Base type of 2nd parameter mirror_conf has been changed from > struct rte_eth_vmdq_mirror_conf to struct rte_eth_mirror_conf." > - Effect: "Replacement of parameter base type may indicate a change in i= ts > semantic meaning." >=20 > So, I'm not sure whether this patch meet the ABI policy? >=20 > Additional, about the validate-abi.sh, does it mean we need to fix all th= e > problems it reports? Or we can decide case by case. Can a Low Severity > problem be acceptable? >=20 > Look forward to your reply. >=20 > Thanks >=20 > Jingjing >=20 > > -----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 > > > > rename rte_eth_vmdq_mirror_conf to rte_eth_mirror_conf and move the > > maximum rule id check from ethdev level to driver > > > > Signed-off-by: Jingjing Wu > > --- > > 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(-) > > > > 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; > > > > - memset(&mr_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); > > + memset(&mr_conf, 0, sizeof(struct rte_eth_mirror_conf)); > > > > - unsigned int vlan_list[ETH_VMDQ_MAX_VLAN_FILTERS]; > > + unsigned int vlan_list[ETH_MIRROR_MAX_VLANS]; > > > > mr_conf.dst_pool =3D res->dstpool_id; > > > > @@ -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; > > > > - 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, > > } > > > > 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; > > > > - 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; > > > > 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); > > > > /* 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(struct 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, > > > > 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); > > > > if (ixgbe_vmdq_mode_check(hw) < 0) > > - return (-ENOTSUP); > > + return -ENOTSUP; > > + > > + if (rule_id >=3D IXGBE_MAX_MIRROR_RULES) > > + return -EINVAL; > > > > /* 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); > > > > memset(&mr_info->mr_conf[rule_id], 0, > > - sizeof(struct rte_eth_vmdq_mirror_conf)); > > + sizeof(struct rte_eth_mirror_conf)); > > > > /* 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]; > > }; > > > > +#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*/ }; > > > > 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, > > > > 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, > > > > 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; > > } > > > > @@ -3062,13 +3062,6 @@ rte_eth_mirror_rule_set(uint8_t port_id, > > return -EINVAL; > > } > > > > - 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); > > > > @@ -3085,13 +3078,6 @@ rte_eth_mirror_rule_reset(uint8_t port_id, > > uint8_t rule_id) > > return -ENODEV; > > } > > > > - 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); > > > > 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. */ > > > > -/* 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 > > > > #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]; > > }; > > > > /** > > * 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; > > }; > > > > /** > > @@ -1211,7 +1212,7 @@ typedef int (*eth_set_vf_rate_limit_t)(struct > > rte_eth_dev *dev, /**< @internal Set VF TX rate */ > > > > 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); > > > > -- > > 1.9.3