From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v4 66/70] bus/fslmc: enable support for mem event callbacks for vfio Date: Mon, 9 Apr 2018 17:39:55 +0530 Message-ID: References: <65d1fceb-91d2-0f8a-9dc2-2530f06cccce@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: "Burakov, Anatoly" Return-path: Received: from EUR03-DB5-obe.outbound.protection.outlook.com (mail-eopbgr40089.outbound.protection.outlook.com [40.107.4.89]) by dpdk.org (Postfix) with ESMTP id 11A491B751 for ; Mon, 9 Apr 2018 13:54:44 +0200 (CEST) In-Reply-To: <65d1fceb-91d2-0f8a-9dc2-2530f06cccce@intel.com> Content-Language: en-US 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 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.