From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 8129C1B813 for ; Mon, 9 Apr 2018 14:35:25 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Apr 2018 05:35:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,427,1517904000"; d="scan'208";a="215150512" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.17.130]) ([10.252.17.130]) by orsmga005.jf.intel.com with ESMTP; 09 Apr 2018 05:35:23 -0700 To: Shreyansh Jain Cc: dev@dpdk.org References: <65d1fceb-91d2-0f8a-9dc2-2530f06cccce@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Mon, 9 Apr 2018 13:35:22 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 66/70] bus/fslmc: enable support for mem event callbacks for vfio 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: Mon, 09 Apr 2018 12:35:26 -0000 On 09-Apr-18 1:09 PM, Shreyansh Jain wrote: > On Monday 09 April 2018 04:25 PM, Burakov, Anatoly wrote: >> On 09-Apr-18 11:01 AM, Shreyansh Jain wrote: >>> Hi Anatoly, >>> >>> On Monday 09 April 2018 01:48 AM, Anatoly Burakov wrote: >>>> VFIO needs to map and unmap segments for DMA whenever they >>>> become available or unavailable, so register a callback for >>>> memory events, and provide map/unmap functions. >>>> >>>> Signed-off-by: Shreyansh Jain >>>> Signed-off-by: Anatoly Burakov >>>> --- >> >> <...> >> >>>> +        DPAA2_BUS_DEBUG("Calling with type=%d, va=%p, >>>> virt_addr=0x%" PRIx64 ", iova=0x%" PRIx64 ", map_len=%zu\n", >>> >>> I would like to correct this message (80char + rewording) - What do >>> you suggest? Should I send a new patch to you or just convey what >>> should be changed? >>> >> >> As far as i know, leaving strings on single line is good for grepping. >> However, perhaps having PRIx64 etc in there breaks it anyway. > > Yes, that and the debug message was not helpful. > This is what I had in mind. (DPAA2_BUS_DEBUG doesn't require an extra \n) > > DPAA2_BUS_DEBUG("Request for %s, va=%p, virt_addr=0x%" PRIx64 "," >         "iova=0x%" PRIx64 ", map_len=%zu", >         type == RTE_MEM_EVENT_ALLOC? "alloc" : "dealloc", >         va, virt_addr, iova_addr, map_len); > >> >>>> +                type, va, virt_addr, iova_addr, map_len); >>>> + >>>> +        if (type == RTE_MEM_EVENT_ALLOC) >>>> +            ret = fslmc_map_dma(virt_addr, iova_addr, map_len); >>>> +        else >>>> +            ret = fslmc_unmap_dma(virt_addr, iova_addr, map_len); >>>> + >>>> +        if (ret != 0) { >>>> +            DPAA2_BUS_ERR("DMA Mapping/Unmapping failed. Map=%d, >>>> addr=%p, len=%zu, err:(%d)", >>>> +                    type, va, map_len, ret); >>> >>> Same as above. 80 Char issue. >> >> Same reasoning - leaving strings unbroken allows for easier grepping >> the codebase, but i'm not sure what's our policy on having formatted >> strings unbroken. > > My policy is not different, but the various variables being dumped > cannot anyway help in grepping - So, keeping the variables on separate > lines for 80chars is ok. "DMA Mapping/Unmapping failed." is enough for > greps. > >> >>> >>>> +            return; >>>> +        } >>>> + >>>> +        cur_len += map_len; >>>> +    } >>>> + >>>> +    if (type == RTE_MEM_EVENT_ALLOC) >>>> +        DPAA2_BUS_DEBUG("Total Mapped: addr=%p, len=%zu\n", >>>> +                addr, len); >>>> +    else >> >> <...> >> >>>> +    ret = rte_mem_event_callback_register("fslmc_memevent_clb", >>>> +                          fslmc_memevent_cb); >>>> +    if (ret && rte_errno == ENOTSUP) >>>> +        DPAA2_BUS_DEBUG("Memory event callbacks not supported"); >>>> +    else if (ret) >>>> +        DPAA2_BUS_DEBUG("Unable to install memory handler"); >>>> +    else >>>> +        DPAA2_BUS_DEBUG("Installed memory callback handler"); >>>>       /* Verifying that at least single segment is available */ >>>>       if (i <= 0) { >>>> +        /* TODO: Is this verification required any more? What would >>>> +         * happen to non-legacy case where nothing was preallocated >>>> +         * thus causing i==0? >>>> +         */ >>> >>> And this as well - if call backs are not going to appear in case of >>> legacy, this needs to be removed. >> >> Callbacks aren't only not going to appear in legacy mode - they will >> also not appear on FreeBSD. We check this above, with checking >> rte_errno value (if callbacks are not supported, it's set to ENOTSUP, >> and having callbacks unsupported is not an error). >> >>> let me know how do you want to take these changes. >>> >> >> Now that i think of it, this error condition is wrong. This is called >> in both legacy and non-legacy mode. This is bus probe, no? For >> non-legacy mode, it is entirely possible to start without any memory >> whatsoever. It just so happens that rte_service API allocates some on >> init, and hence you always have at least one segment - that may not be >> the case forever. So, non-legacy mode, not having memsegs is not an >> error, it is expected behavior, so maybe we should remove this error >> check altogether. > > Agree - that count was only required in the earlier case. It can be > removed. > >> >>>>           DPAA2_BUS_ERR("No Segments found for VFIO Mapping"); >>>> +        rte_rwlock_read_unlock(mem_lock); >>>>           return -1; >>>>       } >>>>       DPAA2_BUS_DEBUG("Total %d segments found.", i); >>>> @@ -250,6 +367,11 @@ int rte_fslmc_vfio_dmamap(void) >>>>        */ >>>>       vfio_map_irq_region(&vfio_group); >>>> +    /* Existing segments have been mapped and memory callback for >>>> hotplug >>>> +     * has been installed. >>>> +     */ >>>> +    rte_rwlock_read_unlock(mem_lock); >>>> + >>>>       return 0; >>>>   } >>>> >>> >>> >> >> > > I think there are enough changes, even if trivial. Maybe I can rework > this patch and send you. If that is inconvenient, just extract from that > whatever you want. > There aren't a lot of changes so i'll respin it myself, addressing the comments above. Thanks! -- Thanks, Anatoly