[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
On Wed, Jul 08, 2015 at 10:31:15AM +, Mcnamara, John wrote: > > -Original Message- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon > > Sent: Tuesday, July 7, 2015 3:51 PM > > To: Wu, Jingjing > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename > > rte_eth_vmdq_mirror_conf > > > > > 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 problem be acceptable? > > > > We have to decide case by case. > > It makes ABI checking impossible to automate. > > That's why any help is welcome to check the git HEAD for ABI violation. > > Hi, > > Automated testing for ABI breaks is tricky since it requires some > interpretation of the errors and the severity level. > > However, each report file contains a 2 line summary at the top that can be > read and parsed. For example (with long lines wrapped): > > head -2 > compat_reports/librte_mbuf.so/v2.0.0_to_ABI_CHECK/compat_report.html > > > > > This still requires interpretation but it can be used to search for > categories of incompatibility. > > To test if a patchset breaks the API it is possible to do something like the > following. Say, for example that you have committed 3 commits to your local > master, you could tag and run the checker like this: > > git checkout master > > git tag -f ABI_CHECK_AFTER > git tag -f ABI_CHECK_BEFORE HEAD~3 > > ./scripts/validate-abi.sh ABI_CHECK_BEFORE ABI_CHECK_AFTER > x86_64-native-linuxapp-gcc > > grep -lr "kind:binary;verdict:incompatible" compat_reports/ > > The grep could be extended to match other categories that you are interested > in. However, it is still probably better to examine the reports manually. > > You can also use something this to run a git bisect to find a commit that > introduced an ABI incompatibility: > > # Tag the head and some known good point in the history. > git checkout master > git tag -f ABI_CHECK_END > git tag -f ABI_CHECK_START v2.0.0 > > # Start git bisect with bad and good. > git bisect start ABI_CHECK_END ABI_CHECK_START > > # Run bisect with a script. > git bisect run ~/abi_bisect.sh > > Where the ~/abi_bisect.sh script that searches for ABI incompatibilities > looks like this: > > #!/bin/sh > > git tag -f ABI_CHECK_END > > rm -rf compat_reports/ > > ./scripts/validate-abi.sh ABI_CHECK_START ABI_CHECK_END > x86_64-native-linuxapp-gcc > > ! grep -lr "kind:binary;verdict:incompatible" compat_reports/ > > John. > -- I'd mod this post up as "+1 informative", except that this isn't slashdot. :-) Very helpful, John. Thanks. /Bruce
[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon > Sent: Tuesday, July 7, 2015 3:51 PM > To: Wu, Jingjing > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename > rte_eth_vmdq_mirror_conf > > > 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 problem be acceptable? > > We have to decide case by case. > It makes ABI checking impossible to automate. > That's why any help is welcome to check the git HEAD for ABI violation. Hi, Automated testing for ABI breaks is tricky since it requires some interpretation of the errors and the severity level. However, each report file contains a 2 line summary at the top that can be read and parsed. For example (with long lines wrapped): head -2 compat_reports/librte_mbuf.so/v2.0.0_to_ABI_CHECK/compat_report.html This still requires interpretation but it can be used to search for categories of incompatibility. To test if a patchset breaks the API it is possible to do something like the following. Say, for example that you have committed 3 commits to your local master, you could tag and run the checker like this: git checkout master git tag -f ABI_CHECK_AFTER git tag -f ABI_CHECK_BEFORE HEAD~3 ./scripts/validate-abi.sh ABI_CHECK_BEFORE ABI_CHECK_AFTER x86_64-native-linuxapp-gcc grep -lr "kind:binary;verdict:incompatible" compat_reports/ The grep could be extended to match other categories that you are interested in. However, it is still probably better to examine the reports manually. You can also use something this to run a git bisect to find a commit that introduced an ABI incompatibility: # Tag the head and some known good point in the history. git checkout master git tag -f ABI_CHECK_END git tag -f ABI_CHECK_START v2.0.0 # Start git bisect with bad and good. git bisect start ABI_CHECK_END ABI_CHECK_START # Run bisect with a script. git bisect run ~/abi_bisect.sh Where the ~/abi_bisect.sh script that searches for ABI incompatibilities looks like this: #!/bin/sh git tag -f ABI_CHECK_END rm -rf compat_reports/ ./scripts/validate-abi.sh ABI_CHECK_START ABI_CHECK_END x86_64-native-linuxapp-gcc ! grep -lr "kind:binary;verdict:incompatible" compat_reports/ John. --
[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
Thanks for the clarification. Jingjing > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, July 07, 2015 10:51 PM > To: Wu, Jingjing > Cc: dev at dpdk.org; nhorman at tuxdriver.com > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename > rte_eth_vmdq_mirror_conf > > 2015-06-26 07:03, Wu, Jingjing: > > Hi, Neil > > > > 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 severity 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 its semantic meaning." > > > > So, I'm not sure whether this patch meet the ABI policy? > > I think it's OK. > > > 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 problem be acceptable? > > We have to decide case by case. > It makes ABI checking impossible to automate. > That's why any help is welcome to check the git HEAD for ABI violation.
[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
2015-06-26 07:03, Wu, Jingjing: > Hi, Neil > > 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 severity 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 its semantic meaning." > > So, I'm not sure whether this patch meet the ABI policy? I think it's OK. > 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 problem be acceptable? We have to decide case by case. It makes ABI checking impossible to automate. That's why any help is welcome to check the git HEAD for ABI violation.
[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
Hi, Thomas Any suggestions about this patch? Do I need rework it by using macro RTE_NEXT_ABI? Thanks a lot! Jingjing > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wu, Jingjing > Sent: Friday, June 26, 2015 3:03 PM > To: 'nhorman at tuxdriver.com' > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename > rte_eth_vmdq_mirror_conf > > Hi, Neil > > 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 severity > 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 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 > problem be acceptable? > > Look forward to your reply. > > Thanks > > Jingjing > > > -Original Message- > > From: Wu, Jingjing > > Sent: Wednesday, June 10, 2015 2:25 PM > > To: dev at 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 = parsed_result; > > - struct rte_eth_vmdq_mirror_conf mr_conf; > > + struct rte_eth_mirror_conf mr_conf; > > > > - memset(_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); > > + memset(_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 = 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 = ETH_VMDQ_VLAN_MIRROR; > > nb_item = parse_item_list(res->value, "core", > > - > > ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1); > > + ETH_MIRROR_MAX_VLANS, vlan_list, > > 1); > > if (nb_item <= 0) > > return; > > > > - for(i=0; i < nb_item; i++) { > > + for (i = 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 = rte_eth_mirror_rule_set(res->port_id,_conf, > > + ret = rte_eth_mirror_rule_set(res->port_id, _conf, > > res->rule_id, 1); > > else > > - ret = rte_eth_mirror_rule_set(res->port_id,_conf, > > + ret = rte_eth_mirror_rule_set(res->port_id, _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 cm
[dpdk-dev] [PATCH v4 1/4] ethdev: rename rte_eth_vmdq_mirror_conf
Hi, Neil 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 severity 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 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 problem be acceptable? Look forward to your reply. Thanks Jingjing > -Original Message- > From: Wu, Jingjing > Sent: Wednesday, June 10, 2015 2:25 PM > To: dev at 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 = parsed_result; > - struct rte_eth_vmdq_mirror_conf mr_conf; > + struct rte_eth_mirror_conf mr_conf; > > - memset(_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); > + memset(_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 = 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 = ETH_VMDQ_VLAN_MIRROR; > nb_item = parse_item_list(res->value, "core", > - > ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1); > + ETH_MIRROR_MAX_VLANS, vlan_list, > 1); > if (nb_item <= 0) > return; > > - for(i=0; i < nb_item; i++) { > + for (i = 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 = rte_eth_mirror_rule_set(res->port_id,_conf, > + ret = rte_eth_mirror_rule_set(res->port_id, _conf, > res->rule_id, 1); > else > - ret = rte_eth_mirror_rule_set(res->port_id,_conf, > + ret = rte_eth_mirror_rule_set(res->port_id, _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 = parsed_result; > - struct rte_eth_vmdq_mirror_conf mr_conf; > + struct rte_eth_mirror_conf mr_conf; > > - memset(_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); > + memset(_conf, 0, sizeof(struct rte_eth_mirror_conf)); > if(!strcmp(res->what, "uplink-mirror")) { > mr_conf.rule_type_mask = 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 = res->dstpool_id; > > if(!strcmp(res->on, "on")) > - ret = rte_eth_mirror_rule_set(res->port_id,_conf, > + ret = rte_eth_mirror_rule_set(res->port_id, _conf, > res->rule_id, 1); > else > - ret = rte_eth_mirror_rule_set(res->port_id,_conf, > + ret = rte_eth_mirror_rule_set(res->port_id, _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
[dpdk-dev] [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 = parsed_result; - struct rte_eth_vmdq_mirror_conf mr_conf; + struct rte_eth_mirror_conf mr_conf; - memset(_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); + memset(_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 = 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 = ETH_VMDQ_VLAN_MIRROR; nb_item = parse_item_list(res->value, "core", - ETH_VMDQ_MAX_VLAN_FILTERS,vlan_list,1); + ETH_MIRROR_MAX_VLANS, vlan_list, 1); if (nb_item <= 0) return; - for(i=0; i < nb_item; i++) { + for (i = 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 = rte_eth_mirror_rule_set(res->port_id,_conf, + ret = rte_eth_mirror_rule_set(res->port_id, _conf, res->rule_id, 1); else - ret = rte_eth_mirror_rule_set(res->port_id,_conf, + ret = rte_eth_mirror_rule_set(res->port_id, _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 = parsed_result; - struct rte_eth_vmdq_mirror_conf mr_conf; + struct rte_eth_mirror_conf mr_conf; - memset(_conf,0,sizeof(struct rte_eth_vmdq_mirror_conf)); + memset(_conf, 0, sizeof(struct rte_eth_mirror_conf)); if(!strcmp(res->what, "uplink-mirror")) { mr_conf.rule_type_mask = 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 = res->dstpool_id; if(!strcmp(res->on, "on")) - ret = rte_eth_mirror_rule_set(res->port_id,_conf, + ret = rte_eth_mirror_rule_set(res->port_id, _conf, res->rule_id, 1); else - ret = rte_eth_mirror_rule_set(res->port_id,_conf, + ret = rte_eth_mirror_rule_set(res->port_id, _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