| Message ID | 1566915962-5472-1-git-send-email-arybchenko@solarflare.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D42B21C1A0; Tue, 27 Aug 2019 16:26:32 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 2727F1C129 for <dev@dpdk.org>; Tue, 27 Aug 2019 16:26:20 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us5.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id F0CC940070; Tue, 27 Aug 2019 14:26:15 +0000 (UTC) Received: from ocex03.SolarFlarecom.com (10.20.40.36) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 27 Aug 2019 07:26:12 -0700 Received: from opal.uk.solarflarecom.com (10.17.10.1) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1395.4 via Frontend Transport; Tue, 27 Aug 2019 07:26:12 -0700 Received: from ukv-loginhost.uk.solarflarecom.com (ukv-loginhost.uk.solarflarecom.com [10.17.10.39]) by opal.uk.solarflarecom.com (8.13.8/8.13.8) with ESMTP id x7REQBL3004160; Tue, 27 Aug 2019 15:26:11 +0100 Received: from ukv-loginhost.uk.solarflarecom.com (localhost [127.0.0.1]) by ukv-loginhost.uk.solarflarecom.com (Postfix) with ESMTP id 33D4F1613D1; Tue, 27 Aug 2019 15:26:11 +0100 (BST) From: Andrew Rybchenko <arybchenko@solarflare.com> To: Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit <ferruh.yigit@intel.com> CC: <dev@dpdk.org> Date: Tue, 27 Aug 2019 15:25:11 +0100 Message-ID: <1566915962-5472-1-git-send-email-arybchenko@solarflare.com> X-Mailer: git-send-email 1.8.3.1 MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24872.005 X-TM-AS-Result: No-2.795400-4.000000-10 X-TMASE-MatchedRID: brcgNj42p6MjdXbalIVNEomfV7NNMGm+p56t1EEQ41r5+tteD5RzhfDM YDo1FKKMl5jUibwqZW7f5+rBd45Ozf2RvKEqDomU/ccgt/EtX/0vLncLLBofLBHfiujuTbedcHK RPTYj/V2vasMDJihmN+v+lktBrsyxFcUL/pOAHTI1VHP4fCovglrdKD8oHqzdT+vTN2DjWP5now nszl3HHVDFjfD7KlTY3YIZ4QpDKxACTWFTFKstBdI0pcl7Mi9m1QG0j/gcakRQNEak+3Lf4wopP RAvyzqMTsQICs9JKZAZ20xSiOCpB5rjPXzSqsiQqJSK+HSPY+9NejdAw/bX35soi2XrUn/Jn6Kd MrRsL14qtq5d3cxkNSYdmvHpWXgM0pEVlB8FopEk22+0HGcHEhBqKVvhqXsUU7oHoWMt8GBi6QO Dz5U7oaqIjgdCvPgRtPpQUocUna2Tw+N7jey4X9aTxr7l5BRrOKBkFAm8GOUPoO5ncI6OuehbQ2 QpmASdWPKWiAlNtI7vdCUIFuasqw== X-TM-AS-User-Approved-Sender: No X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10-2.795400-4.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24872.005 X-MDID: 1566915976-lW_9erl5rqH9 Subject: [dpdk-dev] [PATCH 00/51] ethdev: change rte_eth_dev_info_get() return value to int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
| Series |
ethdev: change rte_eth_dev_info_get() return value to int
|
|
Message
Andrew Rybchenko
Aug. 27, 2019, 2:25 p.m. UTC
It is the first patch series to get rid of void returning functions in ethdev in accordance with deprecation notice [1]. The patch to change dev_infos_get callback prototype to allow driver return error will follow. [1] https://patches.dpdk.org/patch/56969/ Ivan Ilchenko (51): ethdev: change rte_eth_dev_info_get() return value to int app/testpmd: check status of getting ethdev info app/eventdev: check status of getting ethdev info kni: check status of getting ethdev info latency: check status of getting ethdev info pdump: check status of getting ethdev info ring: check status of getting ethdev info app/procinfo: check status of getting ethdev info app/test: check status of getting ethdev info in bonding app/test: check ethdev info get result in event Rx adapter net/bnxt: check status of getting ethdev info net/bonding: check status of getting ethdev info net/netvsc: check status of getting ethdev info net/softnic: check status of getting ethdev info examples/rxtx_callbacks: check status of getting ethdev info examples/l3fwd: check status of getting ethdev info examples/qos_meter: check status of getting ethdev info examples/ip_frag: check status of getting ethdev info examples/performance-thread: check dev info get result examples/vmdq: check status of getting ethdev info examples/distributor: check status of getting ethdev info examples/l3fwd-acl: check status of getting ethdev info examples/vm_power: check status of getting ethdev info examples/qos_sched: check status of getting ethdev info examples/flow_filtering: check status of getting ethdev info examples/l3fwd-power: check status of getting ethdev info examples/l2fwd: check status of getting ethdev info examples/skeleton: check status of getting ethdev info examples/vmdq_dcb: check status of getting ethdev info examples/ipv4_multicast: check status of getting ethdev info examples/l2fwd-jobstats: check status of getting ethdev info examples/bond: check status of getting ethdev info examples/eventdev: check status of getting ethdev info examples/ip_reassembly: check status of getting ethdev info examples/vhost: check status of getting ethdev info examples/ptpclient: check status of getting ethdev info examples/link_status_interrupt: check dev info get result examples/tep_termination: check dev info get result examples/server_node_efd: check dev info get result examples/flow_classify: check status of getting ethdev info examples/packet_ordering: check dev info get result examples/l2fwd-crypto: check status of getting ethdev info examples/multi_process: check status of getting ethdev info examples/ipsec-secgw: check status of getting ethdev info examples/netmap_compat: check status of getting ethdev info examples/l2fwd-keepalive: check dev info get result examples/ip_pipeline: check status of getting ethdev info examples/load_balancer: check status of getting ethdev info examples/kni: check status of getting ethdev info examples/ethtool: check status of getting ethdev info examples/exception_path: check status of getting ethdev info app/proc-info/main.c | 15 ++- app/test-eventdev/test_perf_common.c | 8 +- app/test-eventdev/test_pipeline_common.c | 9 +- app/test-pmd/cmdline.c | 119 +++++++++++++++++++---- app/test-pmd/cmdline_flow.c | 5 +- app/test-pmd/config.c | 78 ++++++++++++--- app/test-pmd/parameters.c | 8 +- app/test-pmd/testpmd.c | 30 ++++-- app/test-pmd/testpmd.h | 3 + app/test-pmd/util.c | 28 +++++- app/test/test_event_eth_rx_adapter.c | 4 +- app/test/test_kni.c | 27 +++++- app/test/test_link_bonding_rssconf.c | 33 ++++++- app/test/test_pmd_ring.c | 8 +- doc/guides/rel_notes/deprecation.rst | 1 - doc/guides/rel_notes/release_19_11.rst | 5 +- drivers/net/bnxt/rte_pmd_bnxt.c | 122 +++++++++++++++++++++--- drivers/net/bonding/rte_eth_bond_api.c | 10 +- drivers/net/bonding/rte_eth_bond_pmd.c | 36 ++++++- drivers/net/netvsc/hn_vf.c | 10 +- drivers/net/softnic/rte_eth_softnic_link.c | 5 +- examples/bond/main.c | 14 ++- examples/distributor/main.c | 8 +- examples/ethtool/ethtool-app/main.c | 8 +- examples/ethtool/lib/rte_ethtool.c | 21 +++- examples/eventdev_pipeline/main.c | 8 +- examples/exception_path/main.c | 7 +- examples/flow_classify/flow_classify.c | 8 +- examples/flow_filtering/main.c | 7 +- examples/ip_fragmentation/main.c | 14 ++- examples/ip_pipeline/kni.c | 5 +- examples/ip_pipeline/link.c | 3 +- examples/ip_reassembly/main.c | 7 +- examples/ipsec-secgw/ipsec-secgw.c | 6 +- examples/ipsec-secgw/ipsec.c | 9 +- examples/ipsec-secgw/sa.c | 10 +- examples/ipv4_multicast/main.c | 7 +- examples/kni/main.c | 26 ++++- examples/l2fwd-crypto/main.c | 9 +- examples/l2fwd-jobstats/main.c | 8 +- examples/l2fwd-keepalive/main.c | 8 +- examples/l2fwd/main.c | 8 +- examples/l3fwd-acl/main.c | 22 ++++- examples/l3fwd-power/main.c | 22 ++++- examples/l3fwd/main.c | 14 ++- examples/link_status_interrupt/main.c | 8 +- examples/load_balancer/init.c | 7 +- examples/multi_process/symmetric_mp/main.c | 8 +- examples/netmap_compat/lib/compat_netmap.c | 9 +- examples/packet_ordering/main.c | 8 +- examples/performance-thread/l3fwd-thread/main.c | 15 ++- examples/ptpclient/ptpclient.c | 9 +- examples/qos_meter/main.c | 16 +++- examples/qos_sched/init.c | 8 +- examples/rxtx_callbacks/main.c | 9 +- examples/server_node_efd/server/init.c | 5 +- examples/skeleton/basicfwd.c | 8 +- examples/tep_termination/vxlan_setup.c | 6 +- examples/vhost/main.c | 9 +- examples/vm_power_manager/main.c | 8 +- examples/vmdq/main.c | 16 +++- examples/vmdq_dcb/main.c | 18 +++- lib/librte_ethdev/rte_ethdev.c | 71 ++++++++++---- lib/librte_ethdev/rte_ethdev.h | 6 +- lib/librte_latencystats/rte_latencystats.c | 23 ++++- lib/librte_pdump/rte_pdump.c | 9 +- 66 files changed, 940 insertions(+), 159 deletions(-)
Comments
Andrew Rybchenko <arybchenko@solarflare.com> writes: > It is the first patch series to get rid of void returning functions > in ethdev in accordance with deprecation notice [1]. This is a huge series, and I suggest to combine some of the work, and/or break it up. Additionally, this patch breaks the ring_pmd_autotest unit test, but I didn't bisect it to find out where. > The patch to change dev_infos_get callback prototype to allow driver > return error will follow. > > [1] https://patches.dpdk.org/patch/56969/ > > Ivan Ilchenko (51): > ethdev: change rte_eth_dev_info_get() return value to int > app/testpmd: check status of getting ethdev info > app/eventdev: check status of getting ethdev info > kni: check status of getting ethdev info > latency: check status of getting ethdev info > pdump: check status of getting ethdev info > ring: check status of getting ethdev info > app/procinfo: check status of getting ethdev info > app/test: check status of getting ethdev info in bonding > app/test: check ethdev info get result in event Rx adapter > net/bnxt: check status of getting ethdev info > net/bonding: check status of getting ethdev info > net/netvsc: check status of getting ethdev info > net/softnic: check status of getting ethdev info > examples/rxtx_callbacks: check status of getting ethdev info > examples/l3fwd: check status of getting ethdev info > examples/qos_meter: check status of getting ethdev info > examples/ip_frag: check status of getting ethdev info > examples/performance-thread: check dev info get result > examples/vmdq: check status of getting ethdev info > examples/distributor: check status of getting ethdev info > examples/l3fwd-acl: check status of getting ethdev info > examples/vm_power: check status of getting ethdev info > examples/qos_sched: check status of getting ethdev info > examples/flow_filtering: check status of getting ethdev info > examples/l3fwd-power: check status of getting ethdev info > examples/l2fwd: check status of getting ethdev info > examples/skeleton: check status of getting ethdev info > examples/vmdq_dcb: check status of getting ethdev info > examples/ipv4_multicast: check status of getting ethdev info > examples/l2fwd-jobstats: check status of getting ethdev info > examples/bond: check status of getting ethdev info > examples/eventdev: check status of getting ethdev info > examples/ip_reassembly: check status of getting ethdev info > examples/vhost: check status of getting ethdev info > examples/ptpclient: check status of getting ethdev info > examples/link_status_interrupt: check dev info get result > examples/tep_termination: check dev info get result > examples/server_node_efd: check dev info get result > examples/flow_classify: check status of getting ethdev info > examples/packet_ordering: check dev info get result > examples/l2fwd-crypto: check status of getting ethdev info > examples/multi_process: check status of getting ethdev info > examples/ipsec-secgw: check status of getting ethdev info > examples/netmap_compat: check status of getting ethdev info > examples/l2fwd-keepalive: check dev info get result > examples/ip_pipeline: check status of getting ethdev info > examples/load_balancer: check status of getting ethdev info > examples/kni: check status of getting ethdev info > examples/ethtool: check status of getting ethdev info > examples/exception_path: check status of getting ethdev info > > app/proc-info/main.c | 15 ++- > app/test-eventdev/test_perf_common.c | 8 +- > app/test-eventdev/test_pipeline_common.c | 9 +- > app/test-pmd/cmdline.c | 119 +++++++++++++++++++---- > app/test-pmd/cmdline_flow.c | 5 +- > app/test-pmd/config.c | 78 ++++++++++++--- > app/test-pmd/parameters.c | 8 +- > app/test-pmd/testpmd.c | 30 ++++-- > app/test-pmd/testpmd.h | 3 + > app/test-pmd/util.c | 28 +++++- > app/test/test_event_eth_rx_adapter.c | 4 +- > app/test/test_kni.c | 27 +++++- > app/test/test_link_bonding_rssconf.c | 33 ++++++- > app/test/test_pmd_ring.c | 8 +- > doc/guides/rel_notes/deprecation.rst | 1 - > doc/guides/rel_notes/release_19_11.rst | 5 +- > drivers/net/bnxt/rte_pmd_bnxt.c | 122 +++++++++++++++++++++--- > drivers/net/bonding/rte_eth_bond_api.c | 10 +- > drivers/net/bonding/rte_eth_bond_pmd.c | 36 ++++++- > drivers/net/netvsc/hn_vf.c | 10 +- > drivers/net/softnic/rte_eth_softnic_link.c | 5 +- > examples/bond/main.c | 14 ++- > examples/distributor/main.c | 8 +- > examples/ethtool/ethtool-app/main.c | 8 +- > examples/ethtool/lib/rte_ethtool.c | 21 +++- > examples/eventdev_pipeline/main.c | 8 +- > examples/exception_path/main.c | 7 +- > examples/flow_classify/flow_classify.c | 8 +- > examples/flow_filtering/main.c | 7 +- > examples/ip_fragmentation/main.c | 14 ++- > examples/ip_pipeline/kni.c | 5 +- > examples/ip_pipeline/link.c | 3 +- > examples/ip_reassembly/main.c | 7 +- > examples/ipsec-secgw/ipsec-secgw.c | 6 +- > examples/ipsec-secgw/ipsec.c | 9 +- > examples/ipsec-secgw/sa.c | 10 +- > examples/ipv4_multicast/main.c | 7 +- > examples/kni/main.c | 26 ++++- > examples/l2fwd-crypto/main.c | 9 +- > examples/l2fwd-jobstats/main.c | 8 +- > examples/l2fwd-keepalive/main.c | 8 +- > examples/l2fwd/main.c | 8 +- > examples/l3fwd-acl/main.c | 22 ++++- > examples/l3fwd-power/main.c | 22 ++++- > examples/l3fwd/main.c | 14 ++- > examples/link_status_interrupt/main.c | 8 +- > examples/load_balancer/init.c | 7 +- > examples/multi_process/symmetric_mp/main.c | 8 +- > examples/netmap_compat/lib/compat_netmap.c | 9 +- > examples/packet_ordering/main.c | 8 +- > examples/performance-thread/l3fwd-thread/main.c | 15 ++- > examples/ptpclient/ptpclient.c | 9 +- > examples/qos_meter/main.c | 16 +++- > examples/qos_sched/init.c | 8 +- > examples/rxtx_callbacks/main.c | 9 +- > examples/server_node_efd/server/init.c | 5 +- > examples/skeleton/basicfwd.c | 8 +- > examples/tep_termination/vxlan_setup.c | 6 +- > examples/vhost/main.c | 9 +- > examples/vm_power_manager/main.c | 8 +- > examples/vmdq/main.c | 16 +++- > examples/vmdq_dcb/main.c | 18 +++- > lib/librte_ethdev/rte_ethdev.c | 71 ++++++++++---- > lib/librte_ethdev/rte_ethdev.h | 6 +- > lib/librte_latencystats/rte_latencystats.c | 23 ++++- > lib/librte_pdump/rte_pdump.c | 9 +- > 66 files changed, 940 insertions(+), 159 deletions(-)
On 8/27/19 11:47 PM, Aaron Conole wrote: > Andrew Rybchenko <arybchenko@solarflare.com> writes: > >> It is the first patch series to get rid of void returning functions >> in ethdev in accordance with deprecation notice [1]. > This is a huge series, and I suggest to combine some of the work, and/or > break it up. I can send patches for examples separately, but it will not help a lot. I can squash changes in examples, but I think it is wrong since it would make review harder - different maintainers and different practices to handle error in different examples (and we tried to take it into account). Other ideas? > Additionally, this patch breaks the ring_pmd_autotest unit test, but I > didn't bisect it to find out where. Many thanks, we'll take a look.
Andrew Rybchenko <arybchenko@solarflare.com> writes: > On 8/27/19 11:47 PM, Aaron Conole wrote: >> Andrew Rybchenko <arybchenko@solarflare.com> writes: >> >>> It is the first patch series to get rid of void returning functions >>> in ethdev in accordance with deprecation notice [1]. >> This is a huge series, and I suggest to combine some of the work, and/or >> break it up. > > I can send patches for examples separately, but it will not help a lot. > I can squash changes in examples, but I think it is wrong since it would > make review harder - different maintainers and different practices to > handle error in different examples (and we tried to take it into account). Hrrm? Not sure what you mean. Patches should be broken up by logical change. That way, it is easy to bisect and isolate what has changed. This series, it seems like there's a single logical change, and that's been spread over lots of patches. I think grouping all the examples and all the app/test together, would make the series 14 review-able patches. As it is, stepping through 40+ 10-line emails is much more tedious (not to mention needing to apply them, check each for build, etc). > Other ideas? > >> Additionally, this patch breaks the ring_pmd_autotest unit test, but I >> didn't bisect it to find out where. > > Many thanks, we'll take a look. This is actually what I'm more concerned about anyway. Please do address this.
On 8/28/19 4:42 PM, Aaron Conole wrote: > Andrew Rybchenko <arybchenko@solarflare.com> writes: > >> On 8/27/19 11:47 PM, Aaron Conole wrote: >>> Andrew Rybchenko <arybchenko@solarflare.com> writes: >>> >>>> It is the first patch series to get rid of void returning functions >>>> in ethdev in accordance with deprecation notice [1]. >>> This is a huge series, and I suggest to combine some of the work, and/or >>> break it up. >> I can send patches for examples separately, but it will not help a lot. >> I can squash changes in examples, but I think it is wrong since it would >> make review harder - different maintainers and different practices to >> handle error in different examples (and we tried to take it into account). > Hrrm? Not sure what you mean. I mean that it is easier to review many small patches than one huge patch especially when these files are maintained by different people. > Patches should be broken up by logical change. That way, it is easy to > bisect and isolate what has changed. This series, it seems like there's > a single logical change, and that's been spread over lots of patches. Single huge patch is worse for both bisect and review. When patch is huge and bisect says that the patch is guilty, you still need to find out which snippet/change is guilty. In this particular case nothing prevents to split the patch make it easier to review and bisect. > I think grouping all the examples and all the app/test together, would > make the series 14 review-able patches. As it is, stepping through 40+ > 10-line emails is much more tedious (not to mention needing to apply > them, check each for build, etc). Yes, less build cycles are required for smaller number of patches, but anyway automation does (should do) it. So, not that important. I disagree that it is easier to review one huge patch. Sorry. I think it is important here that different examples are maintained by different people. Anyway if more reviewers will support the idea to squash examples into once patch, technically it is trial to do. >> Other ideas? >> >>> Additionally, this patch breaks the ring_pmd_autotest unit test, but I >>> didn't bisect it to find out where. >> Many thanks, we'll take a look. > This is actually what I'm more concerned about anyway. Please do > address this.
28/08/2019 16:29, Andrew Rybchenko: > On 8/28/19 4:42 PM, Aaron Conole wrote: > > Andrew Rybchenko <arybchenko@solarflare.com> writes: > >> On 8/27/19 11:47 PM, Aaron Conole wrote: > >>> Andrew Rybchenko <arybchenko@solarflare.com> writes: > >>> > >>>> It is the first patch series to get rid of void returning functions > >>>> in ethdev in accordance with deprecation notice [1]. > >>> This is a huge series, and I suggest to combine some of the work, and/or > >>> break it up. > >> I can send patches for examples separately, but it will not help a lot. > >> I can squash changes in examples, but I think it is wrong since it would > >> make review harder - different maintainers and different practices to > >> handle error in different examples (and we tried to take it into account). > > Hrrm? Not sure what you mean. > > I mean that it is easier to review many small patches than one huge patch > especially when these files are maintained by different people. > > > Patches should be broken up by logical change. That way, it is easy to > > bisect and isolate what has changed. This series, it seems like there's > > a single logical change, and that's been spread over lots of patches. > > Single huge patch is worse for both bisect and review. When patch is huge > and bisect says that the patch is guilty, you still need to find out which > snippet/change is guilty. > > In this particular case nothing prevents to split the patch make it easier > to review and bisect. > > > I think grouping all the examples and all the app/test together, would > > make the series 14 review-able patches. As it is, stepping through 40+ > > 10-line emails is much more tedious (not to mention needing to apply > > them, check each for build, etc). > > Yes, less build cycles are required for smaller number of patches, but > anyway automation does (should do) it. So, not that important. > > I disagree that it is easier to review one huge patch. Sorry. > I think it is important here that different examples are maintained > by different people. Anyway if more reviewers will support the idea > to squash examples into once patch, technically it is trial to do. When doing same kind of change on multiple applications, splitting patches won't help any bisect (they are all different applications anyway). And I think squashing can better show the idea that every applications got the same kind of change (thanks for that). In general, I think patches deserve to be split when there is something interesting to say in the commit log. If you want to describe a different logic of each app, why not. The other way is to explain some exceptions for some applications in a signle patch. My conclusion is that it is often hard to find the good balance, between split and squash, and I can understand any motivation :) Sometimes I squash some patches on apply after they got all reviewed separately. In this case, I didn't look yet :)
On 8/29/2019 6:05 PM, Thomas Monjalon wrote: > 28/08/2019 16:29, Andrew Rybchenko: >> On 8/28/19 4:42 PM, Aaron Conole wrote: >>> Andrew Rybchenko <arybchenko@solarflare.com> writes: >>>> On 8/27/19 11:47 PM, Aaron Conole wrote: >>>>> Andrew Rybchenko <arybchenko@solarflare.com> writes: >>>>> >>>>>> It is the first patch series to get rid of void returning functions >>>>>> in ethdev in accordance with deprecation notice [1]. >>>>> This is a huge series, and I suggest to combine some of the work, and/or >>>>> break it up. >>>> I can send patches for examples separately, but it will not help a lot. >>>> I can squash changes in examples, but I think it is wrong since it would >>>> make review harder - different maintainers and different practices to >>>> handle error in different examples (and we tried to take it into account). >>> Hrrm? Not sure what you mean. >> >> I mean that it is easier to review many small patches than one huge patch >> especially when these files are maintained by different people. >> >>> Patches should be broken up by logical change. That way, it is easy to >>> bisect and isolate what has changed. This series, it seems like there's >>> a single logical change, and that's been spread over lots of patches. >> >> Single huge patch is worse for both bisect and review. When patch is huge >> and bisect says that the patch is guilty, you still need to find out which >> snippet/change is guilty. >> >> In this particular case nothing prevents to split the patch make it easier >> to review and bisect. >> >>> I think grouping all the examples and all the app/test together, would >>> make the series 14 review-able patches. As it is, stepping through 40+ >>> 10-line emails is much more tedious (not to mention needing to apply >>> them, check each for build, etc). >> >> Yes, less build cycles are required for smaller number of patches, but >> anyway automation does (should do) it. So, not that important. >> >> I disagree that it is easier to review one huge patch. Sorry. >> I think it is important here that different examples are maintained >> by different people. Anyway if more reviewers will support the idea >> to squash examples into once patch, technically it is trial to do. > > When doing same kind of change on multiple applications, > splitting patches won't help any bisect (they are all different > applications anyway). And I think squashing can better show the idea that > every applications got the same kind of change (thanks for that). > In general, I think patches deserve to be split when there is something > interesting to say in the commit log. If you want to describe a > different logic of each app, why not. The other way is to explain > some exceptions for some applications in a signle patch. > > My conclusion is that it is often hard to find the good balance, > between split and squash, and I can understand any motivation :) > Sometimes I squash some patches on apply after they got all reviewed > separately. In this case, I didn't look yet :) > I would like to get this early if possible, before end of this week when I expect there will be a peak of patches before the deadline. Overall patchset looks good to me, only one comment on the testpmd one, it also passes automated builds/checks (including patch by patch builds). (ring_pmd_autotest needs to be fixed of course) For the patchset, I agree with Andrew's argument, yes logically same change and apps/examples/tests can be merged into single commit but it would be big & harder to review. Since it is already split, for me it looks good as it is.
On 8/29/19 8:05 PM, Thomas Monjalon wrote: > 28/08/2019 16:29, Andrew Rybchenko: >> On 8/28/19 4:42 PM, Aaron Conole wrote: >>> Andrew Rybchenko <arybchenko@solarflare.com> writes: >>>> On 8/27/19 11:47 PM, Aaron Conole wrote: >>>>> Andrew Rybchenko <arybchenko@solarflare.com> writes: >>>>> >>>>>> It is the first patch series to get rid of void returning functions >>>>>> in ethdev in accordance with deprecation notice [1]. >>>>> This is a huge series, and I suggest to combine some of the work, and/or >>>>> break it up. >>>> I can send patches for examples separately, but it will not help a lot. >>>> I can squash changes in examples, but I think it is wrong since it would >>>> make review harder - different maintainers and different practices to >>>> handle error in different examples (and we tried to take it into account). >>> Hrrm? Not sure what you mean. >> I mean that it is easier to review many small patches than one huge patch >> especially when these files are maintained by different people. >> >>> Patches should be broken up by logical change. That way, it is easy to >>> bisect and isolate what has changed. This series, it seems like there's >>> a single logical change, and that's been spread over lots of patches. >> Single huge patch is worse for both bisect and review. When patch is huge >> and bisect says that the patch is guilty, you still need to find out which >> snippet/change is guilty. >> >> In this particular case nothing prevents to split the patch make it easier >> to review and bisect. >> >>> I think grouping all the examples and all the app/test together, would >>> make the series 14 review-able patches. As it is, stepping through 40+ >>> 10-line emails is much more tedious (not to mention needing to apply >>> them, check each for build, etc). >> Yes, less build cycles are required for smaller number of patches, but >> anyway automation does (should do) it. So, not that important. >> >> I disagree that it is easier to review one huge patch. Sorry. >> I think it is important here that different examples are maintained >> by different people. Anyway if more reviewers will support the idea >> to squash examples into once patch, technically it is trial to do. > When doing same kind of change on multiple applications, > splitting patches won't help any bisect (they are all different > applications anyway). And I think squashing can better show the idea that > every applications got the same kind of change (thanks for that). > In general, I think patches deserve to be split when there is something > interesting to say in the commit log. If you want to describe a > different logic of each app, why not. The other way is to explain > some exceptions for some applications in a signle patch. Thanks a lot, makes sense and I agree with explanations. The only problem is review of such huge patches, when reviewer needs to find out his snippet. Also split version is better for patchwork reviewed/acked counters. It is clear which snippets are reviewed, which are not. Unfortunately split does not help this patchset to get more attention from reviewers :( > My conclusion is that it is often hard to find the good balance, > between split and squash, and I can understand any motivation :) > Sometimes I squash some patches on apply after they got all reviewed > separately. In this case, I didn't look yet :)
Andrew Rybchenko <arybchenko@solarflare.com> writes: > On 8/29/19 8:05 PM, Thomas Monjalon wrote: > > 28/08/2019 16:29, Andrew Rybchenko: > > On 8/28/19 4:42 PM, Aaron Conole wrote: > > Andrew Rybchenko <arybchenko@solarflare.com> writes: > > On 8/27/19 11:47 PM, Aaron Conole wrote: > > Andrew Rybchenko <arybchenko@solarflare.com> writes: > > It is the first patch series to get rid of void returning functions > in ethdev in accordance with deprecation notice [1]. > > This is a huge series, and I suggest to combine some of the work, and/or > break it up. > > I can send patches for examples separately, but it will not help a lot. > I can squash changes in examples, but I think it is wrong since it would > make review harder - different maintainers and different practices to > handle error in different examples (and we tried to take it into account). > > Hrrm? Not sure what you mean. > > > I mean that it is easier to review many small patches than one huge patch > especially when these files are maintained by different people. > > Patches should be broken up by logical change. That way, it is easy to > bisect and isolate what has changed. This series, it seems like there's > a single logical change, and that's been spread over lots of patches. > > > Single huge patch is worse for both bisect and review. When patch is huge > and bisect says that the patch is guilty, you still need to find out which > snippet/change is guilty. > > In this particular case nothing prevents to split the patch make it easier > to review and bisect. > > I think grouping all the examples and all the app/test together, would > make the series 14 review-able patches. As it is, stepping through 40+ > 10-line emails is much more tedious (not to mention needing to apply > them, check each for build, etc). > > > Yes, less build cycles are required for smaller number of patches, but > anyway automation does (should do) it. So, not that important. > > I disagree that it is easier to review one huge patch. Sorry. > I think it is important here that different examples are maintained > by different people. Anyway if more reviewers will support the idea > to squash examples into once patch, technically it is trial to do. > > > When doing same kind of change on multiple applications, > splitting patches won't help any bisect (they are all different > applications anyway). And I think squashing can better show the idea that > every applications got the same kind of change (thanks for that). > In general, I think patches deserve to be split when there is something > interesting to say in the commit log. If you want to describe a > different logic of each app, why not. The other way is to explain > some exceptions for some applications in a signle patch. > > Thanks a lot, makes sense and I agree with explanations. > The only problem is review of such huge patches, when > reviewer needs to find out his snippet. At least, hopefully the reviewer knows from the changestat what to look for to narrow it down. > Also split version > is better for patchwork reviewed/acked counters. It is > clear which snippets are reviewed, which are not. > Unfortunately split does not help this patchset to get > more attention from reviewers :( That's a problem for which I have no solution. If you solve it please let me know how. :) > My conclusion is that it is often hard to find the good balance, > between split and squash, and I can understand any motivation :) > Sometimes I squash some patches on apply after they got all reviewed > separately. In this case, I didn't look yet :)