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 B8E60A0540; Mon, 20 Jul 2020 14:20:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8C6FE1DBB; Mon, 20 Jul 2020 14:20:48 +0200 (CEST) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by dpdk.org (Postfix) with ESMTP id 3B4E01023 for ; Mon, 20 Jul 2020 14:20:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1595247646; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wgWTy+6gNrx5yYrZL8b1U8v8aq3jlvfXwfraCjMND5o=; b=ZiBYgcKwEOvqPpe4LQm9CPN4WFMp8rGGsiA1UqkKy9rSVWhB2wxVCmw23owAqGIn3QLMUZ 5ChoxEorXUo34YdIRuvzcImI8kefxMJ7cYX0Mha2xQvYo4Ew7fWHO61A3PhHw2kNr3jLh1 +OPqUSMp1Agzkc3lnaitC49XLRZaZt0= Received: from mail-vs1-f70.google.com (mail-vs1-f70.google.com [209.85.217.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-63-zBP9XzSDNSCcExqZ6JyUdQ-1; Mon, 20 Jul 2020 08:20:44 -0400 X-MC-Unique: zBP9XzSDNSCcExqZ6JyUdQ-1 Received: by mail-vs1-f70.google.com with SMTP id d15so2890538vsr.14 for ; Mon, 20 Jul 2020 05:20:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wgWTy+6gNrx5yYrZL8b1U8v8aq3jlvfXwfraCjMND5o=; b=bkUh9kNREO3wRoF/orclreQJeFIf32IlvqwvIiw6QPyhjJk9/ztOJfsWnBINUM3FUR 4p+WKpZ1/NyWr5IMM5E9JQPRjmaQ5MoteavKD0dny7f2UCVlLzvvdqs/oJOFHIibF2GO rDNbBIYFOC/U5NZgYixusl+bsEMG/O7UjRmMi2SZaT4YVCNfABBHaA9BZN4EyaKfWGgg JRwnWmfKTLgXZLSn55x6hgKPXznBakybJ33jo4tZ9s5k5xAmRlVHlE3PIDZG5rc0tkYr iny0lMdXqN3JNAezaf2JirHi9fjCrfJczpiHF59hHJ7qmVzF5R6+zQy0kfQZycptXfRJ 3QAw== X-Gm-Message-State: AOAM5339JDWivf4sRWo3HxkYw/sUO2ye5xaiNJRYoADVKLCTE1gSjZaO /r6I9vjU3fYZaHBiSDK5fMGiubA/OUE4pUJQRPHKOYhUwKBrA70bNDdRh8Z4L50ispVqG2rsjeO nbl8BrejXVukrtN26fUQ= X-Received: by 2002:ab0:6950:: with SMTP id c16mr15585460uas.53.1595247644196; Mon, 20 Jul 2020 05:20:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxtHld3fN23mlsGiFhVNHOZORzB47OAdskT85OfFnIPWUgGjzra7iL7w96zQleJOJbzVWkrK1DY9gAjvpCRj1k= X-Received: by 2002:ab0:6950:: with SMTP id c16mr15585434uas.53.1595247643816; Mon, 20 Jul 2020 05:20:43 -0700 (PDT) MIME-Version: 1.0 References: <1591870283-7776-1-git-send-email-radu.nicolau@intel.com> <1595236337-28230-1-git-send-email-radu.nicolau@intel.com> <1595236337-28230-2-git-send-email-radu.nicolau@intel.com> In-Reply-To: <1595236337-28230-2-git-send-email-radu.nicolau@intel.com> From: David Marchand Date: Mon, 20 Jul 2020 14:20:32 +0200 Message-ID: To: Radu Nicolau Cc: dev , Beilei Xing , Jeff Guo , Bruce Richardson , "Ananyev, Konstantin" , Jerin Jacob , "Trahe, Fiona" , Wei Zhao , "Ruifeng Wang (Arm Technology China)" Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v9 1/4] eal: add WC store functions 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 Mon, Jul 20, 2020 at 11:12 AM Radu Nicolau wrote: > > Add rte_write32_wc and rte_write32_wc_relaxed functions > that implement 32bit stores using write combining memory protocol. > Provided generic stubs and x86 implementation. What is the difference of using this new API when compared to the existing pci driver flag RTE_PCI_DRV_WC_ACTIVATE? Do we have some overlap between the two? This commitlog is quite short for something that touches performance. I saw a question from Ruifeng, it is worth adding this to the commitlog. Which x86 platforms will benefit from it? What is the impact on performance for existing platforms that have no MOVDIRI support? > > Signed-off-by: Radu Nicolau > Acked-by: Bruce Richardson > --- > lib/librte_eal/arm/include/rte_io_64.h | 12 +++++++ > lib/librte_eal/include/generic/rte_io.h | 48 ++++++++++++++++++++++++++++ > lib/librte_eal/x86/include/rte_io.h | 56 +++++++++++++++++++++++++++++++++ > 3 files changed, 116 insertions(+) > > diff --git a/lib/librte_eal/arm/include/rte_io_64.h b/lib/librte_eal/arm/include/rte_io_64.h > index e534624..d07d9cb 100644 > --- a/lib/librte_eal/arm/include/rte_io_64.h > +++ b/lib/librte_eal/arm/include/rte_io_64.h > @@ -164,6 +164,18 @@ rte_write64(uint64_t value, volatile void *addr) > rte_write64_relaxed(value, addr); > } > > +static __rte_always_inline void > +rte_write32_wc(uint32_t value, volatile void *addr) > +{ > + rte_write32(value, addr); > +} > + > +static __rte_always_inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) > +{ > + rte_write32_relaxed(value, addr); > +} > + We were using a single knob RTE_OVERRIDE_IO_H for overriding the whole rte_io.h. Now we would have a special case for an API for x86 and the code is copy/pasted in the ARM header and keeping the "whole" override mode. This leaves an unfinished taste. Why did you not flag all relevant "native" helpers? This would factor some code from the ARM header. > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_eal/include/generic/rte_io.h b/lib/librte_eal/include/generic/rte_io.h > index da457f7..0669baa 100644 > --- a/lib/librte_eal/include/generic/rte_io.h > +++ b/lib/librte_eal/include/generic/rte_io.h > @@ -229,6 +229,40 @@ rte_write32(uint32_t value, volatile void *addr); > static inline void > rte_write64(uint64_t value, volatile void *addr); > > +/** > + * Write a 32-bit value to I/O device memory address addr using write > + * combining memory write protocol. Depending on the platform write combining > + * may not be available and/or may be treated as a hint and the behavior may > + * fallback to a regular store. > + * > + * @param value > + * Value to write > + * @param addr > + * I/O memory address to write the value to > + */ > +__rte_experimental > +static inline void > +rte_write32_wc(uint32_t value, volatile void *addr); > + > +/** > + * Write a 32-bit value to I/O device memory address addr using write > + * combining memory write protocol. Depending on the platform write combining > + * may not be available and/or may be treated as a hint and the behavior may > + * fallback to a regular store. > + * > + * The relaxed version does not have additional I/O memory barrier, useful in > + * accessing the device registers of integrated controllers which implicitly > + * strongly ordered with respect to memory access. It might be just me, but I have trouble reading the last part of this sentence. Maybe remove "with respect to"? > + * > + * @param value > + * Value to write > + * @param addr > + * I/O memory address to write the value to > + */ > +__rte_experimental > +static inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr); > + > #endif /* __DOXYGEN__ */ > > #ifndef RTE_OVERRIDE_IO_H > @@ -345,6 +379,20 @@ rte_write64(uint64_t value, volatile void *addr) > rte_write64_relaxed(value, addr); > } > > +#ifndef RTE_NATIVE_WRITE32_WC > +static __rte_always_inline void > +rte_write32_wc(uint32_t value, volatile void *addr) > +{ > + rte_write32(value, addr); > +} > + > +static __rte_always_inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) > +{ > + rte_write32_relaxed(value, addr); > +} > +#endif /* RTE_NATIVE_WRITE32_WC */ > + > #endif /* RTE_OVERRIDE_IO_H */ > > #endif /* _RTE_IO_H_ */ > diff --git a/lib/librte_eal/x86/include/rte_io.h b/lib/librte_eal/x86/include/rte_io.h > index 2db71b1..c95ed67 100644 > --- a/lib/librte_eal/x86/include/rte_io.h > +++ b/lib/librte_eal/x86/include/rte_io.h > @@ -9,8 +9,64 @@ > extern "C" { > #endif > > +#include "rte_cpuflags.h" > + > +#define RTE_NATIVE_WRITE32_WC > #include "generic/rte_io.h" > > +/** > + * @internal > + * MOVDIRI wrapper. > + */ > +static __rte_always_inline void > +_rte_x86_movdiri(uint32_t value, volatile void *addr) > +{ > + asm volatile( > + /* MOVDIRI */ > + ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02" > + : > + : "a" (value), "d" (addr)); > +} > + > +static __rte_always_inline void > +rte_write32_wc(uint32_t value, volatile void *addr) > +{ > + static int _x86_movdiri_flag = -1; > + if (_x86_movdiri_flag == 1) { > + rte_wmb(); > + _rte_x86_movdiri(value, addr); > + } else if (_x86_movdiri_flag == 0) { > + rte_write32(value, addr); > + } else { > + _x86_movdiri_flag = > + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); > + if (_x86_movdiri_flag == 1) { > + rte_wmb(); > + _rte_x86_movdiri(value, addr); > + } else { > + rte_write32(value, addr); > + } > + } > +} > + > +static __rte_always_inline void > +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) > +{ > + static int _x86_movdiri_flag = -1; > + if (_x86_movdiri_flag == 1) { > + _rte_x86_movdiri(value, addr); > + } else if (_x86_movdiri_flag == 0) { > + rte_write32_relaxed(value, addr); > + } else { > + _x86_movdiri_flag = > + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); > + if (_x86_movdiri_flag == 1) > + _rte_x86_movdiri(value, addr); > + else > + rte_write32_relaxed(value, addr); > + } > +} > + Repeating some comments I made earlier. - If a single helper called by both rte_write32_wc and rte_write32_wc_relaxed with a _constant_ flag is not to your liking (I don't see where it would have an impact on performance), then maybe rte_write32_wc() can be simply implemented as: +static __rte_always_inline void +rte_write32_wc(uint32_t value, volatile void *addr) +{ + rte_wmb(); + rte_write32_wc_relaxed(value, addr); +} + - Looking at this above suggestion, I wonder about the non-relaxed case. Is rte_io_wmb() not enough? - The cpuflag check can be resolved at init once and for all. By this, I mean in lib/librte_eal/x86/rte_cpuflags.c: +int rte_x86_movdiri_flag = -1; + +RTE_INIT(rte_x86_movdiri_init) +{ + rte_x86_movdiri_flag = + (rte_cpu_get_flag_enabled(RTE_CPUFLAG_MOVDIRI) > 0); +} The variable can be exported in lib/librte_eal/x86/include/rte_cpuflags.h. Then rte_write32_wc_relaxed() becomes: +static __rte_always_inline void +rte_write32_wc_relaxed(uint32_t value, volatile void *addr) +{ + if (rte_x86_movdiri_flag == 1) { + asm volatile( + /* MOVDIRI */ + ".byte 0x40, 0x0f, 0x38, 0xf9, 0x02" + : + : "a" (value), "d" (addr)); + return; + } + + rte_write32_relaxed(value, addr); +} + Thanks. -- David Marchand