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 CF90DA057C; Thu, 26 Mar 2020 18:42:23 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BECDD1C029; Thu, 26 Mar 2020 18:42:22 +0100 (CET) Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) by dpdk.org (Postfix) with ESMTP id 47CD01C028 for ; Thu, 26 Mar 2020 18:42:21 +0100 (CET) Received: by mail-lf1-f66.google.com with SMTP id q5so5588862lfb.13 for ; Thu, 26 Mar 2020 10:42:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=mxfvt81e0+xyQmoinIO917h+alGusEnDGRM+nbSVfsk=; b=wt4Y5Hos7qsi2n9+oGr2XGrpkLgPc7deDFbC/tRW54cTsEIR4HeYKzMZZwQY2tcmmA nJdLXsc00rZ75Ky0619vRWqbm0NGS1yxjpflutIipymZxYIYcb3hqFsvetdfCWnEXFOx quln2PFqhND5ELPopBoYpZKAcDkgh00TrrodPhgCfQIy68FISfac/e7zdcha/DcSHE88 O7h4SPxGvX0Og5Hzn/5vkZp6KDL4o0iUNp5Y+fMoDCHiQs8SpswgJI/VAcHd4kVPKfR9 xkctN63ciNSxbQoJfIQREm2HiYfGriRwkmJ+7AG1IJa1/+0qtX+6K8xuNW3L+Z1KUaiz f3rQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=mxfvt81e0+xyQmoinIO917h+alGusEnDGRM+nbSVfsk=; b=J/XdRp9edrg/TK9IfpI9WuBADtHwmTHskp86lYC11XogpR0VeDWHE2PW8lSzn8r2Sa 3wn3eOcvt9eFi/y7WOQtBLJYE4ifN6bg+Ocvbvi/5FDdyf/BDCLVP6gCdCImi9KRGRXq q3yz2BcjlOAiu9qzzrsOK3a8+pDglnzcYAK5crcpQj2o2IE+MAx+2G8o4FOlzaLNoGhe 8pOPFLNxi09DsWWXS7OsPIvoVhCLROi0C5gU+F/ytjTGbCyhJQhA1PWjm1MZpE2piFpq 2QvSxw283slarGisol7pwx0rvn/VTpsGExxjWhzQgVjBTS3BfsXCqmtCcrEej9YrImWp 4Ijw== X-Gm-Message-State: AGi0PuaOXUii91z49UVd+mj/4QkrCvNZwMfhicNizVMmK7yZF6dgbIOR 6fULieL2eWs4aBs5944cJFfl4/LIEQYkDg== X-Google-Smtp-Source: APiQypIK/ZqAwEupS2qvIfRPzL2uT0Nd+nAiAcGWzxklgbcjDQJK50/7PFP/PGincsGtpGIEKaerHA== X-Received: by 2002:ac2:5208:: with SMTP id a8mr1154618lfl.88.1585244539845; Thu, 26 Mar 2020 10:42:19 -0700 (PDT) Received: from [192.168.8.100] (user-5-173-56-20.play-internet.pl. [5.173.56.20]) by smtp.gmail.com with ESMTPSA id i18sm2020965lfe.15.2020.03.26.10.42.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Mar 2020 10:42:19 -0700 (PDT) To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Andrzej Ostruszka , David Marchand Cc: dev References: <20200306164104.15528-1-aostruszka@marvell.com> <20200310111037.31451-1-aostruszka@marvell.com> <98CBD80474FA8B44BF855DF32C47DC35C60EFD@smartserver.smartshare.dk> From: Andrzej Ostruszka Message-ID: <7886ce41-84eb-c7c0-fa4d-cd3a44825630@semihalf.com> Date: Thu, 26 Mar 2020 18:42:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C60EFD@smartserver.smartshare.dk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 0/4] Introduce IF proxy library 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 3/25/20 12:11 PM, Morten Brørup wrote: [...] >>> - Notification scheme has been changed - instead of having just >>> callbacks now event queueing is also available (or a mix of those >>> two). > > Thank you for adding event queueing! That was actually a good input from you - thank you. > David mentions ABI forward compatibility below. > Consider using a dynamically sized generic TLV (type, length, value) > message format instead of a big union structure for the events. This > would make it easier to extend the list of event types without breaking > the ABI. My understanding is that David was talking about registering of callbacks and you want to extend this to event definition. So let's focus on one example: ... RTE_IFPX_NEIGH_ADD, RTE_IFPX_NEIGH_DEL, ... struct rte_ifpx_neigh_change { uint16_t port_id; struct rte_ether_addr mac; uint32_t ip; }; Right now the event is defined as: struct rte_ifpx_event { enum rte_ifpx_event_type type; union { ... struct rte_ifpx_neigh_change neigh_change; ... }; }; So what the user does is a switch on event->type: switch (ev->type) { case RTE_IFPX_NEIGH_ADD: handle_neigh_add(lconf, &ev->neigh_change); break; case RTE_IFPX_NEIGH_DEL: handle_neigh_del(lconf, &ev->neigh_change); break; How does adding more event types to this union would break ABI? User gets event from the queue (allocated by the lib) checks the type and casts the pointer past the 'type' to proper event definition. And when done with the event simply free()s it (BTW right now it is malloc() not rte_malloc() - should I change that?). If app links against newer version of lib then it might get type which it does not understand/handle so it should skip (possibly with a warning). I'm not sure how changing rte_ifpx_event to: struct rte_ifpx_event { enut rte_ifpx_event_type type; int length; uint8_t data[]; }; would help here. The user would need to cast data based on event type whereas now it takes address of a proper union member - and the union is there only to avoid casting. In both cases what is important is that RTE_IFPX_NEIGH_ADD/DEL and "struct rte_ifpx_neigh_change" don't change between versions (new values can be added - or new versions of the previously existing events when trying to make a change). And for the callbacks it is more or less the same - library will prepare data and call callback with a pointer to this data. Handling of new event types should be automatic when I implement what David wanted - simply lib callback for the new event will be NULL nothing will be called and application will work without problems. > And I am still strongly opposed to the callback method: Noted - however for now I would like to keep them. I don't have much experience with this library so if they prove to be inadequate then we will remove them. Right now they seem to add some flexibility that I like: - if something should be changed globally and once (and it is safe to do so!) then it can be done from the callback - if something can be prepared once and consumed later by lcores then it can be done in callback and the callback returns 0 so that event is still queued and lcores (under assumption that queues are per lcore) pick up what has been prepared. > The callbacks are handled as DPDK interrupts, which are running in a non-DPDK > thread, i.e. a running callback may be preempted by some other Linux process. > This makes it difficult to implement callbacks correctly. > The risk of someone calling a non-thread safe function from a callback is high, > e.g. DPDK hash table manipulation (except lookup) is not thread safe. > > Your documentation is far too vague about this: > Please note however that the context in which these callbacks are > called is most probably different from the one in which packets are > handled and it is application writer responsibility to use proper > synchronization mechanisms - if they are needed. > > You need a big fat WARNING about how difficult the DPDK interrupt thread is to > work with. As I described above, it is not "most probably" it is "certainly" a > very different kind of context. OK. Will update in next version. > Did you check that the functions you use in your example callbacks are all > thread safe and non-blocking, so they can safely be called from a non-DPDK thread > that may be preempted by a another Linux process? I believe so. However there is a big question whether my assumption about LPM is correct. I've looked at the code and it looks like it so but I'm not in power to authoritatively declare it. So again, to me LPM looks like safe to be changed by a single writer while being used by multiple readers (with an obvious transient period when rule is being expanded and some IPs might go with an old and some with a new destination). With regards Andrzej Ostruszka