From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B4551A2E1B for ; Tue, 3 Sep 2019 14:27:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 96E071DFEA; Tue, 3 Sep 2019 14:27:39 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 206391D454 for ; Tue, 3 Sep 2019 14:27:38 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us2.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 625E868006B; Tue, 3 Sep 2019 12:27:35 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 3 Sep 2019 13:27:29 +0100 To: Thomas Monjalon , Aaron Conole CC: Ferruh Yigit , "dev@dpdk.org" , "Ivan Ilchenko" References: <1566915962-5472-1-git-send-email-arybchenko@solarflare.com> <660e1152-9ae5-8698-5e1f-bca45943cfd9@solarflare.com> <13169824.Gphp0D0tZG@xps> From: Andrew Rybchenko Message-ID: <66023bf0-b49d-452e-d6a0-f9cb84cab86e@solarflare.com> Date: Tue, 3 Sep 2019 15:27:25 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <13169824.Gphp0D0tZG@xps> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24886.003 X-TM-AS-Result: No-19.270800-8.000000-10 X-TMASE-MatchedRID: DuKherWvI/s4Q7K/HmUF53bspjK6JP6qJuDBbd4NSqQNWA0aQ3FdiCGU b2JNxi1q2RIa0u5oeaZOYuXMsEGdZb9z4PlYm1BmiS8eKdD/7uTkJIYDKPF738uSXx71bvSLVtO 0mkb0wEE+NfMMkHtdnHn08LPIYQ+CaBevM/eurMfQfyKEYQc1R0irUEQnqkg0ycmFNidOeD3Ljq rNNKG3txW+ozhHCHFaHiGEYM6sI0WeH1x0y0x/SlLFinzH90WnYwrduvUxofgELMPQNzyJS+EMl /vI3P9nM22M43riQ2rqBW43j0nGZsq+KpbekBWYSHCU59h5KrHKhROoEXUTm7jOUXWmQ3OWU9Qz Xh9MBw4HUJKL6sLeErFsPwK5NF6Q0UJQ8lvowYblV0RgnASGwGCZ4AImwaJ+DjjiM8xEqBM/J5S KnlHdbeRNctgjswXff9E9VgjlcRbcklY9MAaJUgwwf9JuP1zEGUqOjzOC7IpEvDDW7fraa7oxJ7 f0SGm388kx1EV/9JkeDL9JpeZwrw0d6nS+l6UZekmbTWzSjHrCfpS2pVkEUSvFSzw3D/Z+iF03R /4DJgOkRgEg7nrRyq/xMC8mu+5XcejanjZWHJjmuecNOZVsqiD8Ih+V5f0PirEwGuznvmIsWVdl 3YcDsJ19tSqG3E03FAhrH5Kra4M4c5PfPWZTtg0QY5VnQyANHAwy0XhreD35lA8aRaIEF6PFjJE Fr+olA9Mriq0CDAgBi3kqJOK62SAHAopEd76v0tpXMhvkqYzlr0ZMhDVLI11b3BZ+DwaUhtyqpo fquB/Cdc5fjn8byQ== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.270800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24886.003 X-MDID: 1567513656-I4HOI6mY7RbY Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 writes: >>>> On 8/27/19 11:47 PM, Aaron Conole wrote: >>>>> Andrew Rybchenko 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 :)