From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-HE1-obe.outbound.protection.outlook.com (mail-eopbgr10056.outbound.protection.outlook.com [40.107.1.56]) by dpdk.org (Postfix) with ESMTP id 01C6A1B4F0 for ; Fri, 13 Jul 2018 09:07:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=dky41TOKBsvjqaFV7rVFPpivMD3AVJH5PUV7IKjwdhM=; b=mcMY71ECDYwyrQ12JhGgtqwUHLkKqm/xDtbDwGH4Xx2E/+J/Ru06mZFrWUqnuEVa3bswuZze1enABBFWubN4pJgCwZypRIpxuxmLNfDY+TH1LY//8J5Iz+3hG9TvhP+dqNNldljQfi4d/6P+NvS/7xT2yeBZHrHgmRPobt9xdys= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=shreyansh.jain@nxp.com; Received: from [10.232.14.39] (14.142.187.166) by AM0PR04MB4675.eurprd04.prod.outlook.com (2603:10a6:208:75::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.930.21; Fri, 13 Jul 2018 07:07:15 +0000 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Cc: dev@dpdk.org References: <1a96a4a1-71fd-74ef-f599-22dc21134ed4@nxp.com> <20180712150832.lmict7bn3exkyh2u@bidouze.vm.6wind.com> From: Shreyansh Jain Message-ID: <993bb4f2-b73a-ab38-53bf-420b9552f107@nxp.com> Date: Fri, 13 Jul 2018 12:36:50 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 In-Reply-To: <20180712150832.lmict7bn3exkyh2u@bidouze.vm.6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [14.142.187.166] X-ClientProxiedBy: PN1PR0101CA0008.INDPRD01.PROD.OUTLOOK.COM (2603:1096:c00:e::18) To AM0PR04MB4675.eurprd04.prod.outlook.com (2603:10a6:208:75::17) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: d8916822-505b-4573-d6f5-08d5e88f43ec X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989117)(5600053)(711020)(4534165)(7168020)(4627221)(201703031133081)(201702281549075)(8990107)(48565401081)(2017052603328)(7153060)(7193020); SRVR:AM0PR04MB4675; X-Microsoft-Exchange-Diagnostics: 1; AM0PR04MB4675; 3:ObbhlWbjZ6xeuhcxtLnnbpvqeSFvV8pScaf2mrY5g2LKZg8+ktpLUnCsjCKCj4wPI7WTxvvAB3/xPq1uViVlzQt57sQYx5lfAJ7Ue9nV1K1AlGySxVlxjrCSml6xAzXCy/H3vctpoElFi0ezSQ/izVGD2beW6gtAGyG5MMv7ocQMwtcnU1DXYC+BVpgFhs/LyLUSjpUqsnWz72B46+0wRWEAt9iv5jUnCxZlaepvUUBcY9Lch/P64znzYbtMsoMw; 25:Lx/DvIgfq94lMOzS6cXgLuNdAL0nSWRkxfLlU6ymLBDKoMeC8CGf595e3YxfaCNP2c87/ZjJNGnuUoAOC9Ddc0LQhGXM4MZeRMmEno6hT9YB24vXcTJ/Fn6CMhVB+FWmqth9Ru04iwEBeYyiCUVlhMnm/Voxi6oEtEaY9pzkuIrzKTqRN2mSU4M1CkAumq8OQG4uafj1j7Mq1uAVSkh4PnhmiToHbTT6yMk1itZRmE5v83P5zyGvjE24ez5bbEE5j6TIe932tojc8J/RlH+DEvkHKm4+lOoCatZF5kMTIK5+KXUKvWYwGTnyQoyCZhpdvA5zh6kUKzHTgCKlfN2E0Q==; 31:EDvLaHr73l1GKhmZbaRK2Vc9ORs990GHq9YTAhDU9NrcqXUHqq3vcFIYWZaT+8RDEtwoEm8MbBpdSmWJyLEQUVjkM6YQWLt9J9sd9ixI/rInv10jjJti/TBV9iaO5gGC6Rf00afyLB77TP+tyHp1KO9zvq5p1f+rA4doyTTUIKASiZ8S8YqKeM2yqM/I1IzV1wqzjhy8cQnwDLb/VO7tVVzHbgjUoUtjT3OVNNktLKo= X-MS-TrafficTypeDiagnostic: AM0PR04MB4675: X-Microsoft-Exchange-Diagnostics: 1; AM0PR04MB4675; 20:rnThk5uY140FcDC50RXnsHG5uoIEM/VED68k+GW1bGAOrpvv8Hmx50Az3uDAX1GmUxLn0GX9dxUBMUMSBiZDJSBQLn79eZd0Zgmkw8r5pjEhjGuCmqm76PF7XQlX85H5CGGujhQY1whh5Q+DaXrlVfMSZzvfSDusy+aZlP0JOTD4ZQM8BOkUNoU7dbXCz4GxM8gPwHLUTU6ZtkaaesCicpdikEKQ1feKXPl5HXeetWpe4+3GMg8EEmxXf9sBOiC9GdppRhcAjAQ71wv0bU+4+7gRmB84PESjgYMGDGEO9lDDifJlasJKBIq/FbGWn2+BH4tCdGwvBMvxqbifoq607r5UqbNB+PX67vFO5QElsayfVkiW9UQuSL8LwyYbzLY8I64rKz01oofxrhMjF+dWj5sjI4FWYyfORjZ7dk+iPaweUHJejGeznKEi50w734lfMrAg8DrAlyDPcd1CcVbg3dS806Pk4OABvxzfIKjb8XuzSDFCo0SOpj4DdK67BIOi; 4:v8SfBugXM8+SH7yX+LysRS27fiIfLAc3Ka+xx0xp5jEsyj092Yij/29L89UlhJS3oh/J/CvOk8lRB6ZbVsrz/VQztWt+PAwhwkdVDYGMQvHhvHDGOfoIO3LlA0xKFWDb6r4JFWgsUCawwH5z0TfW6kFeu2F8zIN1pJvWA/MhgL5vXGlm3tNfNvs8yLnOF3Q3itcbU9A8b32EWANj7ybJDAGngPjMnTnz1JbU5RtAvrzLRzS0bwI0pZugmEKpFiiaRR1SR+saQ5RGNf3Vcp9S9Q== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-MS-Exchange-SenderADCheck: 1 X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3231311)(944501410)(52105095)(3002001)(6055026)(149027)(150027)(6041310)(20161123560045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123564045)(6072148)(201708071742011)(7699016); SRVR:AM0PR04MB4675; BCL:0; PCL:0; RULEID:; SRVR:AM0PR04MB4675; X-Forefront-PRVS: 07326CFBC4 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(6049001)(376002)(39860400002)(366004)(346002)(396003)(136003)(189003)(199004)(55674003)(81166006)(31696002)(2906002)(76176011)(26005)(50466002)(6116002)(68736007)(2870700001)(6666003)(5660300001)(7736002)(65826007)(81156014)(8676002)(97736004)(66066001)(478600001)(956004)(93886005)(65806001)(305945005)(65956001)(25786009)(5009440100003)(47776003)(3846002)(446003)(2486003)(36756003)(6916009)(53936002)(106356001)(31686004)(105586002)(186003)(486006)(23676004)(6486002)(8936002)(67846002)(16526019)(16576012)(77096007)(11346002)(14444005)(86362001)(229853002)(55236004)(58126008)(44832011)(4326008)(52116002)(2616005)(386003)(476003)(6246003)(64126003)(52146003)(316002)(110426005); DIR:OUT; SFP:1101; SCL:1; SRVR:AM0PR04MB4675; H:[10.232.14.39]; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; Received-SPF: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtBTTBQUjA0TUI0Njc1OzIzOk05Y0gyak45b1EwL3BzREg4cVV4OXVSL3VZ?= =?utf-8?B?T09aOTI3d2dydTZ1QURCemJDNG1zK2Q4Vjg2Rk9ZbTNJRUIzbllNYS9INWFW?= =?utf-8?B?V2d1eUM0Q3dRb2Erbzh2T0x4WDFWYXE0TnpRRW5keFZFN1ZtQTU5WTcrRlhw?= =?utf-8?B?K3FjQ2lsRzVUY3RzWjRndzZKTnZJNCsvN09XaURRdXlVSTVEbGJZSXlUTFlp?= =?utf-8?B?eUk1eUNweEIyejZoUXRrMXFLY3BhaVRLOWJZeUp2WWlHUW5mS3pKbEtBK3dX?= =?utf-8?B?NEFjaEZ3ZEhLdG5KdFNmM1RTaHpxdmxVUXE5bi8yRStUWnMzNFFtWFNtVUVw?= =?utf-8?B?dVJqYmVOazRNRU43MzFoUnFrVCtUREo0QUlWajlVV0Y0WlhpT25FYzdlcWdo?= =?utf-8?B?RllsdkFJWlR2REswY0c3VTNMdkRaeTdOZHRsdmRmS01iQmV2dE5QdUFTay9w?= =?utf-8?B?d2ZWblJvZXZLcmVhUGh5YXdPa1RsdWUvN1BkeEtrejNBUE5CMW8zTlNZdDVa?= =?utf-8?B?Y3pISEpqakhJMFh2NzJKTHFNK09rWGdrc0d2WFpiWGJpenBSaFlxUFA3YVEw?= =?utf-8?B?VkUyT2d2VUdjM1hicmFHaGdEVThHMUxXNm1hcWpvMWMzYkJERm1PbjYxTXBQ?= =?utf-8?B?N0pjdXpjQnQwZ0hDUGM2cExKdk03QmdCOWU0VjNvQVE4TVpPYTBBTGVqL0NY?= =?utf-8?B?MnYwS2tsclo0ZU0vNVBkeDg2VmxXeGN5REhyWjkzelMvMStMUlNDN2FxU1Qx?= =?utf-8?B?WGdrZCtlMkw2NUR2OEZUWk9LbHFvTVdnTm1mQXh0TUszQURyb2QrVXlyU0Fk?= =?utf-8?B?NXgzenFlY2pVWERDaDVtbXpBQ1lwT2pSTUJyTzE4L2JDV0IzTm45KzBndjBx?= =?utf-8?B?bWVEQVllVVdzRXhaYWxFajVmR25jMnRnQTMwQVlKL1J1a21hNkZkMW5wZUUv?= =?utf-8?B?QnllalJUNHdlTzNvMFdhNXd5S0ErUXNGdzRmNjZIVzBKUXY0NG1DZFQrWUlR?= =?utf-8?B?Z1VJZWVBUmxlYUx6TGQrdlkraVIyOCs1TTNicEpjKzVFdkNhbW50OVNPUnlV?= =?utf-8?B?WmtzSG9xY0xhakp0eFhOcXJXdU5CMUltOW5icmNZbXpFU0lwaFc3UVpScmdC?= =?utf-8?B?UFlIbjFqMWo0aXlBRTNKY1E0eDIvRUdTeW5LbXEzbStLVHB4cFZyc3kvdHZN?= =?utf-8?B?cDRMdjIzMUduZHV3OE9zM2NkK0FXOTFPd2dBUmUzcUVzbDVVL0dBb1N1SU9L?= =?utf-8?B?VkRMZ01lYXRoaHJLMzNES256YWxEWDEwUTY4YXBrS21CZ2dLSGFiZzllbnJx?= =?utf-8?B?dmI0akVmQ0Q1VWJpbnV6QVBDbGpRV09nVUp1eVhIeDd2Vlh6bUJoSHBma0p0?= =?utf-8?B?cm5ZR2o0cG5tNGdmSFBoSytmV2JkWWNsUHpkYTFFejdlNk5TaG9pT0hRaTNT?= =?utf-8?B?SnVBMlluMmJGamdkN2tiVWU2OVEvZzNsdkMxTHFjZmZxTG1OOGcvUFo3WkNl?= =?utf-8?B?SE93d0NQV0YxNEs3cVAyd0hIYjJWU3pCb09ZZ2grZzNQZ2Zib2g5WVNZekxJ?= =?utf-8?B?VkJ6TzBZMUlZT3ErOUZPWnAvUUN2SzNmc0dtemdNbWRwZGpESTU5cC9sWk04?= =?utf-8?B?V3YxVDVLcklkaHBxelNIYzdXTnhWcUNyWFZaWWhmcFFXbi9WOW1MMTdaWTND?= =?utf-8?B?TnNTbkw5M1UydmlRNWhLNjdwMk80aGNvOHV3TEZXaHdjRFNMeUJuZFhLTlRP?= =?utf-8?B?Zy9waTA1V2gwaGVOb3FodVdNU0J0TUV0bVhEaytDMndsWkpFWHAxS093N3Mz?= =?utf-8?B?QkVNaThxQ2tBZkFrbzBTS3dSbVpNYXNndTZSdnh1MHhSVk5EeklrRmU1cXpD?= =?utf-8?B?TElLK1JNQ0N5ek5Mc2tKd25NQmQ3UkZyR2RjeTV4QTgwemF4RE1ORUJuNEtp?= =?utf-8?B?QkNGYUdSUDczM0ZtTFlRUENpbEZXL1B0dVRwa3FPeUlrc2gzZ1ZmNVdPV1kv?= =?utf-8?B?YXVoM2Y1SnBGK2FEZ0RoOGJmUTF5eGNsanUrUG1VQzZISk8vL0dOMGw0V3pq?= =?utf-8?B?K1BRanphUlJEYW92SWFTc3pVdnZRWGc0VUVTQndJSVhpb1hzODJZbW5rYUp4?= =?utf-8?Q?BNRkgMcKiv5BJiUo8FMVCxs=3D?= X-Microsoft-Antispam-Message-Info: nEMP8/HW3Nif37rcEjDmwnHoBQbI0hT4mUdKvoQX7WswYzsr9jl0w1x8UU0j46XdK7BdiFAd20Co7UlvYGLXF2MaZcff7IDpd7iQ7Mifva4aofkK+4xl6Z5G5026XvMMmeh9I1jqFgUXwd8oM6ydMa5pfpuBP8V555LMq8t7pUrd4YNlQ3DkqiDGuqztb9fDHjR+5KIbxcjUgWk0vmw6a1oXsvlYx0jKZfDMO+U4cY6/4D/6V7sV5v2MxfEtDprP8hjK+oL7psNjeQNln0Nf6nf8xx1BtJSRA2gfRaG71Xg93rzP4dniGdYUB7X8YaE8CW0l41p0w4Vy7Ysk2opIKv4flaMi7lDiWxJ52lNetLs= X-Microsoft-Exchange-Diagnostics: 1; AM0PR04MB4675; 6:BmevKiJ+C0qYNBdqg73oUCXWGWdsE+WPdPgmNoHa1PEQoDcSOZfIVP7PAigHdgLhrLl6Rz5s/i8U5a9uZhRtaBFfytWxIyUyNAMsBAaJiFqKifj7dy9RJrEW2SLJGQJOMjc7UeAPoad4v5f4YFt+XwhoWocy8HlXz24leofbkdwYP/ztkTkG6SiEg3XYgBg+1RteQ2dK+2KI5nOuRwtvrymPkUJVEurHnTugk6vd/LKT+wscRbfKrwVfy/k6XK1IneDD9/QwObXwMCEHQjzrlV26BX5ik8+gsm4LjFY+4hs6Fqgp2Hy204EHDL3A0K1Uam/EYV/QAIcP9KiORCB5kWs29tblrY4pVNNFoIsQTR8CVrZb/SierVgArUn6EnSYmsO1GIV2JiSiwHOxmPyWl8FZ//YmE9wjcNXxjMrZcq6lovvJEH6aLhf29mlUCnyOWKgp4v81ERNfvnfetZBgrw==; 5:YT/vWQ2d2Cd+Nj0dNZSZD1GPxWSxBcP6bG/EV4B4FGk37GxD50u4o66EyS/avt5LUgYOBdKbYO0aDVNlhhoDcy+oiwd0CWtOnaI1DV61UN8RPBMvKNtzFiWantTbteRIF7MSCOgwNR4ShtC9J/KTQ78s1gsbS6eNdvC0gChTok8=; 24:jEGrx/22iX8BDHsX1e1Cm0yvUj0VDITk9DqWek+Uyh9Ynvk0qJzzOUR4L0lI5gcnCQ2UQssbmFY5jJcFOi8ynXUD8Qc8hHY3T+cdnU0ZtXQ= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; AM0PR04MB4675; 7:KbuXOOcDYklMpunmCqggCU4RAD0ak6Uxyo1H8c44TNRr9H5jq1gg6u61kRQyTF4izUOVZ80DiCklqbydF+4W+0iNbPRwS1eI43sW60w7+kQAWBOmiuUkHBexJtGmoNyIQLDkK0Ceevvb2kT1wl36bKbpZtgVw/omr6dwn/vw2Q7O0oVEP/eGfsXyUSP749Y1ftRQ8kmXegsK7qmDZtb0iifdLfruOKPSciHCZDx5ajEEZJTg8cBkPBbeq7TLSRP0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jul 2018 07:07:15.4261 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: d8916822-505b-4573-d6f5-08d5e88f43ec X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB4675 Subject: Re: [dpdk-dev] [PATCH v11 11/25] eal/dev: implement device iteration 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: , X-List-Received-Date: Fri, 13 Jul 2018 07:07:18 -0000 (I am not reducing the thread as it contains quite interesting discussion - so, you might have to fish for inline comments...) On Thursday 12 July 2018 08:38 PM, Gaëtan Rivet wrote: > Hi Shreyansh, > > On Thu, Jul 12, 2018 at 04:28:27PM +0530, Shreyansh Jain wrote: >> On Thursday 12 July 2018 03:15 AM, Gaetan Rivet wrote: >>> Use the iteration hooks in the abstraction layers to perform the >>> requested filtering on the internal device lists. >>> >>> Signed-off-by: Gaetan Rivet >>> --- >>> lib/librte_eal/common/eal_common_dev.c | 168 ++++++++++++++++++++++++ >>> lib/librte_eal/common/include/rte_dev.h | 26 ++++ >>> lib/librte_eal/rte_eal_version.map | 1 + >>> 3 files changed, 195 insertions(+) >>> >>> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c >>> index 63e329bd8..b78845f02 100644 >>> --- a/lib/librte_eal/common/eal_common_dev.c >>> +++ b/lib/librte_eal/common/eal_common_dev.c >>> @@ -45,6 +45,28 @@ static struct dev_event_cb_list dev_event_cbs; >>> /* spinlock for device callbacks */ >>> static rte_spinlock_t dev_event_lock = RTE_SPINLOCK_INITIALIZER; >>> +struct dev_next_ctx { >>> + struct rte_dev_iterator *it; >>> + const char *bus_str; >>> + const char *cls_str; >>> +}; >>> + >>> +#define CTX(it, bus_str, cls_str) \ >>> + (&(const struct dev_next_ctx){ \ >>> + .it = it, \ >>> + .bus_str = bus_str, \ >>> + .cls_str = cls_str, \ >>> + }) >>> + >>> +#define ITCTX(ptr) \ >>> + (((struct dev_next_ctx *)(intptr_t)ptr)->it) >>> + >>> +#define BUSCTX(ptr) \ >>> + (((struct dev_next_ctx *)(intptr_t)ptr)->bus_str) >>> + >>> +#define CLSCTX(ptr) \ >>> + (((struct dev_next_ctx *)(intptr_t)ptr)->cls_str) >>> + >>> static int cmp_detached_dev_name(const struct rte_device *dev, >>> const void *_name) >>> { >>> @@ -398,3 +420,149 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, >>> get_out: >>> return -rte_errno; >>> } >>> + >>> +static char * >>> +dev_str_sane_copy(const char *str) >>> +{ >>> + size_t end; >>> + char *copy; >>> + >>> + end = strcspn(str, ",/"); >>> + if (str[end] == ',') { >>> + copy = strdup(&str[end + 1]); >>> + } else { >>> + /* '/' or '\0' */ >>> + copy = strdup(""); >>> + } >> >> Though it doesn't change anything functionally, if you can separate blocks >> of if-else with new lines, it really makes it easier to read. >> Like here... >> > > sure, > >>> + if (copy == NULL) { >>> + rte_errno = ENOMEM; >>> + } else { >>> + char *slash; >>> + >>> + slash = strchr(copy, '/'); >>> + if (slash != NULL) >>> + slash[0] = '\0'; >>> + } >>> + return copy; >>> +} >>> + >>> +static int >>> +class_next_dev_cmp(const struct rte_class *cls, >>> + const void *ctx) >>> +{ >>> + struct rte_dev_iterator *it; >>> + const char *cls_str = NULL; >>> + void *dev; >>> + >>> + if (cls->dev_iterate == NULL) >>> + return 1; >>> + it = ITCTX(ctx); >>> + cls_str = CLSCTX(ctx); >>> + dev = it->class_device; >>> + /* it->cls_str != NULL means a class >>> + * was specified in the devstr. >>> + */ >>> + if (it->cls_str != NULL && cls != it->cls) >>> + return 1; >>> + /* If an error occurred previously, >>> + * no need to test further. >>> + */ >>> + if (rte_errno != 0) >>> + return -1; >> >> I am guessing here by '..error occurred previously..' you mean sane_copy. If >> so, why wait until this point to return? Anyway the caller (rte_bus_find, >> probably) would only look for '0' or non-zero. >> > > No, rte_errno could be set by a bus / class implementation, for any > error occurring during a call to dev_iterate: maybe a device was lost > (hotplugged), etc. The return value of dev_iterate() cannot transmit an > error as not matching a filter is not an error. The only error channel > is rte_errno. > > sane_copy was already checked before and should be cleared at this > point. > >>> + dev = cls->dev_iterate(dev, cls_str, it); >>> + it->class_device = dev; >>> + return dev == NULL; >>> +} >>> + >>> +static int >>> +bus_next_dev_cmp(const struct rte_bus *bus, >>> + const void *ctx) >>> +{ >>> + struct rte_device *dev = NULL; >>> + struct rte_class *cls = NULL; >>> + struct rte_dev_iterator *it; >>> + const char *bus_str = NULL; >>> + >>> + if (bus->dev_iterate == NULL) >>> + return 1; >>> + it = ITCTX(ctx); >>> + bus_str = BUSCTX(ctx); >>> + dev = it->device; >>> + /* it->bus_str != NULL means a bus >>> + * was specified in the devstr. >>> + */ >>> + if (it->bus_str != NULL && bus != it->bus) >>> + return 1; >>> + /* If an error occurred previously, >>> + * no need to test further. >>> + */ >>> + if (rte_errno != 0) >>> + return -1; >>> + if (it->cls_str == NULL) { >>> + dev = bus->dev_iterate(dev, bus_str, it); >>> + goto end; >> >> This is slightly confusing. If it->cls_str == NULL, you do >> bus->dev_iterate... but >> >>> + } >>> + /* cls_str != NULL */ >>> + if (dev == NULL) { >>> +next_dev_on_bus: >>> + dev = bus->dev_iterate(dev, bus_str, it); >> >> When cls_str!=NULL, you still do bus->dev_iterate... >> So, maybe they are OR case resulting in check of dev==NULL and return (as >> being done right now by jumping to out)...? >> > > Yes, this iteration is pretty complex. > > The best way to walk through it is to define the possible cases: > > 1. Iterating on one bus: > > if (bus_str != NULL && cls_str == NULL) > > Simplest case. You got one bus, no classes. > Just call the current bus dev_iterate() callback, then report > the result (whether NULL or not). > > 2. Iterating on one bus and one class: > > if (bus_str != NULL && cls_str != NULL) > > Possible states are: > > a. We are starting the iteration: dev is NULL. > Iterate on the current bus. > > If device is not NULL anymore, iterate on the class. > To ensure we stay on the same class, set cls to the previous class > and call rte_class_find(); > > If device is still NULL, return 1 to iterate on the next bus. > > b. We are in the middle of an iteration: dev is not NULL. > We start iterating on the class to find a possible second instance > of the same device in the class (e.g. mlx devices with multiple > eth ports per PCI devices). If none is found, we > come back to bus->dev_iterate() (bypassing the dev == NULL check), > restarting this (b) cycle as many times as necessary. > If the result is NULL, the iteration is finished. > > 3. Iterating on one class: > > if (bus_str == NULL && cls_str != NULL) > > The most complex case. Same as (2), however we start by the first > bus, and if a device is NULL we will continue onto the next bus > within the loop line 554 (within rte_dev_iterator_next). > This, above, is most useful. I had missed out the case (b) above entirely. I will re-review with this in mind. Thanks for writing this. > > The core of the complexity here lies in the next_dev_on_bus cycle > described in 2.b. > >> And, how can bus iterate over a 'null' device? >> > > A NULL device means that we are starting the iteration: a bus will give > its first device. > >>> + it->device = dev; >>> + } >>> + if (dev == NULL) >>> + return 1; >> >> Maybe, this check should move in the if(dev==NULL) above - that way, it can >> in path of 'next_dev_on_bus' yet do the same as function as its current >> location. >> > > Yes > >>> + if (it->cls != NULL) >> >> In what case would (it->cls_str == NULL) but (it->cls != NULL)? >> > > When one rte_device was used to spawn several class_devices: multiple > adapters per PCI addresses for example. > > However, in this case, given that the matching would be useless, we skip > the class matching process and only return the rte_device. This single > rte_device is not returned multiple times. > > However, if someone was giving: > > bus=pci,id=00:02.0/class=eth > (str_sane_copy would set cls_str to "" here) > > Then, if 00:02.0 had spawned several eth ports, it would be returned > once for each instance. > > This is a pretty ambiguous case. I'm not sure of the best way to deal with > it. My decision here was to simplify the iteration if possible, as I > considered that someone that did not care for the class properties would > not care about counting the instances of the bus device. maybe I'm > wrong. Frankly, I have nothing extra to add as a use-case. For your approach of 'simple is better': +1 > >>> + cls = TAILQ_PREV(it->cls, rte_class_list, next); >>> + cls = rte_class_find(cls, class_next_dev_cmp, ctx); >>> + if (cls != NULL) { >>> + it->cls = cls; >>> + goto end; >>> + } >>> + goto next_dev_on_bus; >> >> Maybe I have completely mixed your intention of this function. So, if you >> still find the above comments naive - maybe you can tell me what you are >> attempting here? >> Is it: find next bus and stop if no class specified. find next class as >> well, iff that too was specified? >> >> Reason I am confused is that bus_next_dev_cmp attempts to compare both - bus >> and class, yet class_next_dev_cmp simply stops by comparing class only. >> > > You are right: bus comparator will produce an iteration on the bus > devices, then on the class devices iff class is specified. > > Thus the class comparator only has to iterate on class, because we > called it from the bus iterator. > >>> +end: >>> + it->device = dev; >>> + return dev == NULL; >>> +} >> >> A new line should be added here - start of a new function. >> > > yes > > > I'm pretty sorry about this code, to be honest. > This is a nasty piece with a lot of states to care for. Actually, the cases that you have mentioned above indeed requires a complex logic. I am not sure I have any suggestion to make it simpler. I will read again based on what you have explained. > > At least it works. I'd like to have the properties integrated so that > developpers can add their own quickly. In the meantime I could rework > this function. But simple is not easy. > Problem is that the 18.08 window is almost closed - I though I could review it within the window but now I am not confident. Maybe someone else too can look through the PCI/VDEV part after this patch..