From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 341C31E20 for ; Fri, 13 Jan 2017 15:23:38 +0100 (CET) Received: by mail-wm0-f49.google.com with SMTP id c85so67026861wmi.1 for ; Fri, 13 Jan 2017 06:23:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=NwhgyIVzkVA6Ijrx4Ow1ZG9Zl3LwQJ27Pz9cSYKHa1M=; b=i14pDDYZfSCnz3tEQwyjXioUMhX7pDff8qePrlTIvulBCSSoPi8xDu7goJ4/BXTfUS F2emw46VS/zVOqPnDOMB+/9dnt9r4kNI+PliLUWPShP/0WBQzGFz6LVkF0F6U7OXmgro GjhyLgUaX8EmGSGDVRiF3PEeTc9NFpdGH0afNFoLOv4XScGV8GGKK1/VgH85F0zFLRjB 7EpGN5Ga9qVfXQI6L2mZT7hQx/xCskENDWrtyuMNB9ww9bpvQ0cpweXcYlnU7c+DQcbL iTe2fYzmkUL3WkAzlx4HA5xuwnjQbmE7kKzDoEP/k1pJeOVfqf0hnlbWHXaajd1CpAnH ADbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=NwhgyIVzkVA6Ijrx4Ow1ZG9Zl3LwQJ27Pz9cSYKHa1M=; b=tCFdUrrwDpxT3BJ6qvb0Cnhu+Km4f0a8OqXLLSaD5gYhb4kWjxLe4lEBjlB7RldqiJ CP7eG2IOcoP7MAhVw+doUaQU/+2GOLm2W2Wbz9N/7qrfhBEIRIsPzsDIkFoNFMiinsTS ckD9DNzY0rM94889lSKpkZLxuqCFAzGy4mOETgCDOyV64e2rB/ERd34BhKgrPv52ogL/ hzW0MbXniXxuPzVtuIpNtJdRc+3zo+7j6yL94QLFU6KVmhUqEgmA3Ml8XlJ6tqmNyoO5 vZIlq3pUnlsChUheCe4zAuLzYGMgkZ+yUe5rmrEbewOUlcQrfEcKl5nxEg6KCDgpf4cL xmGQ== X-Gm-Message-State: AIkVDXJHvnuosdu+GdrsLc5SG6Fxng1enV0WRyjbl5I97HMwhBuu2fIqGwal9a/wNc+R61GB X-Received: by 10.28.46.144 with SMTP id u138mr2376076wmu.136.1484317417845; Fri, 13 Jan 2017 06:23:37 -0800 (PST) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id d29sm4592333wmi.19.2017.01.13.06.23.37 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 13 Jan 2017 06:23:37 -0800 (PST) Date: Fri, 13 Jan 2017 15:23:34 +0100 From: Olivier Matz To: Bruce Richardson Cc: dev@dpdk.org Message-ID: <20170113152334.5efda35a@platinum> In-Reply-To: <1484147125-5948-1-git-send-email-bruce.richardson@intel.com> References: <1484147125-5948-1-git-send-email-bruce.richardson@intel.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes 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 Jan 2017 14:23:38 -0000 Hi Bruce, On Wed, 11 Jan 2017 15:05:14 +0000, Bruce Richardson wrote: > The rte_ring library in DPDK provides an excellent high-performance > mechanism which can be used for passing pointers between cores and > for other tasks such as buffering. However, it does have a number > of limitations: > > * type information of pointers is lost, as it works with void pointers > * typecasting is needed when using enqueue/dequeue burst functions, > since arrays of other types cannot be automatically cast to void ** > * the data to be passed through the ring itself must be no bigger than > a pointer > > While the first two limitations are an inconvenience, the final one is > one that can prevent use of rte_rings in cases where their > functionality is needed. The use-case which has inspired the patchset > is that of eventdev. When working with rte_events, each event is a > 16-byte structure consisting of a pointer and some metadata e.g. > priority and type. For these events, what is passed around between > cores is not pointers to events, but the events themselves. This > makes existing rings unsuitable for use by applications working with > rte_events, and also for use internally inside any software > implementation of an eventdev. > > For rings to handle events or other similarly sized structures, e.g. > NIC descriptors, etc., we then have two options - duplicate rte_ring > code to create new ring implementations for each of those types, or > generalise the existing code using macros so that the data type > handled by each rings is a compile time paramter. This patchset takes > the latter approach, and once applied would allow us to add an > rte_event_ring type to DPDK using a header file containing: > > #define RING_TYPE struct rte_event > #define RING_TYPE_NAME rte_event > #include > #undef RING_TYPE_NAME > #undef RING_TYPE > > [NOTE: the event_ring is not defined in this set, since it depends on > the eventdev implementation not present in the main tree] > > If we want to elimiate some of the typecasting on our code when > enqueuing and dequeuing mbuf pointers, an rte_mbuf_ring type can be > similarly created using the same number of lines of code. > > The downside of this generalisation is that the code for the rings now > has far more use of macros in it. However, I do not feel that overall > readability suffers much from this change, the since the changes are > pretty much just search-replace onces. There should also be no ABI > compatibility issues with this change, since the existing rte_ring > structures remain the same. I didn't dive deeply in the patches, just had a quick look. I understand the need, and even if I really don't like the "#define + #include" way to create a new specific ring (for readability, grepability), that may be a solution to your problem. I think using a similar approach than in sys/queue.h would be even worse in terms of readability. What do you think about the following approach? - add a new elt_size in rte_ring structure - update create/enqueue/dequeue/... functions to manage the elt size - change: rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table, unsigned n) to: rte_ring_enqueue_bulk(struct rte_ring *r, void *obj_table, unsigned n) This relaxes the type for the API in the function. In the caller, the type of obj_table would be: - (void **) in case of a ring of pointers - (uint8_t *) in case of a ring of uint8_t - (struct rte_event *) in case of a ring of rte_event ... I think (I have not tested it) it won't break compilation since any type can be implicitly casted into a void *. Also, I'd say it is possible to avoid breaking the ABI. - deprecate or forbid calls to: rte_ring_mp_enqueue(struct rte_ring *r, void *obj) (and similar) Because with a ring of pointers, obj is the pointer, passed by value. For other types, we would need rte_ring_mp_enqueue(struct rte_ring *r, obj) Maybe we could consider using a macro here. The drawbacks I see are: - a dynamic elt_size may slightly decrease performance - it still uses casts to (void *), so there is no type checking Regards, Olivier