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 9E9F1A00E6 for ; Tue, 9 Jul 2019 17:23:38 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7E8501B997; Tue, 9 Jul 2019 17:23:38 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6318C1B970 for ; Tue, 9 Jul 2019 17:23:36 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2019 08:23:35 -0700 X-IronPort-AV: E=Sophos;i="5.63,470,1557212400"; d="scan'208";a="156208895" Received: from dhunt5-mobl4.ger.corp.intel.com (HELO [10.237.221.141]) ([10.237.221.141]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/AES256-SHA; 09 Jul 2019 08:23:32 -0700 To: "Burakov, Anatoly" , dev@dpdk.org Cc: lukaszx.gosiewski@intel.com, Marcin Hajkowski , Lukasz Krakowiak References: <20190613092117.7252-2-marcinx.hajkowski@intel.com> <20190709150753.15732-1-david.hunt@intel.com> From: "Hunt, David" Message-ID: <6458ca4c-2665-b4ad-5b64-aa696e1b699c@intel.com> Date: Tue, 9 Jul 2019 16:23:31 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v5] power: add fifo per core for JSON interface 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" Hi Anatoly, On 09/07/2019 16:12, Burakov, Anatoly wrote: > On 09-Jul-19 4:07 PM, David Hunt wrote: >> From: Marcin Hajkowski >> >> This patch implements a separate FIFO for each cpu core to improve the >> previous functionality where anyone with access to the FIFO could affect >> any core on the system. By using appropriate permissions, fifo >> interfaces >> can be configured to only affect the particular cores. >> >> Because each FIFO is per core, the following fields have been removed >> from the command JSON format: core_list, resource_id, name. >> >> Signed-off-by: Lukasz Krakowiak >> Signed-off-by: Lukasz Gosiewski >> Signed-off-by: Marcin Hajkowski >> Tested-by: David Hunt >> >> --- >> v2: >> * updated handling vm_name (use proper buff size) >> * rebase to master changes >> >> v3: >> * improvement to coding style >> >> v4: >> * rebase to tip of master >> >> v5: >> * merged docs into same patch as the code, as per mailing list policy >> * made changes out of review by Anatoly. >> --- > > > >> -    if (setup_host_channel_info(&chan_info, 0) < 0) { >> -        rte_free(chan_info); >> -        return 0; >> +        ret = fifo_path(socket_path, sizeof(socket_path), i); >> +        if (ret < 0) >> +            goto error; >> + >> +        ret = mkfifo(socket_path, 0660); >> +        RTE_LOG(DEBUG, CHANNEL_MANAGER, "TRY CREATE fifo '%s'\n", >> +            socket_path); >> +        if ((errno != EEXIST) && (ret < 0)) { >> +            RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create fifo '%s' >> error: " >> +                    "%s\n", socket_path, strerror(errno)); >> +            goto error; >> +        } >> +        chan_info = rte_malloc(NULL, sizeof(*chan_info), 0); >> +        if (chan_info == NULL) { >> +            RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating memory >> for " >> +                    "channel '%s'\n", socket_path); >> +            goto error; >> +        } >> +        chan_infos[i] = chan_info; >> +        rte_strlcpy(chan_info->channel_path, socket_path, >> +                sizeof(chan_info->channel_path)); >> + >> +        if (setup_host_channel_info(&chan_info, i) < 0) { >> +            rte_free(chan_info); >> +            chan_infos[i] = NULL; >> +            goto error; >> +        } >> +        num_channels_enabled++; >>       } >> -    num_channels_enabled++; >>         return num_channels_enabled; >> +error: >> +    /* Clean up the channels opened before we hit an error. */ >> +    for (i = 0; i < RTE_MAX_LCORE; i++) { > > You're going up to RTE_MAX_LCORE here, but up to ci->core_count in the > original loop. Is that intentional? > > Other than that, > > Acked-by: Anatoly Burakov > Just to be totally clean, I've fixed the loop limit, and respun as v6. :) Thanks for the review. Rgds, Dave.