From: "Burakov, Anatoly" <anatoly.burakov@intel.com> To: Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org Cc: david.marchand@redhat.com, ferruh.yigit@intel.com, grive@u256.net, alvinx.zhang@intel.com, beilei.xing@intel.com, jia.guo@intel.com, bruce.richardson@intel.com, dmitry.kozliuk@gmail.com, navasile@linux.microsoft.com, dmitrym@microsoft.com, pallavi.kadam@intel.com, talshn@mellanox.com Subject: Re: [dpdk-dev] [PATCH] pci: keep API compatibility with mmap values Date: Fri, 10 Jul 2020 16:39:02 +0100 Message-ID: <3151e427-77af-80c0-e53b-4e107bb1a40c@intel.com> (raw) In-Reply-To: <20200710115324.3902559-1-thomas@monjalon.net> On 10-Jul-20 12:53 PM, Thomas Monjalon wrote: > The function pci_map_resource() returns MAP_FAILED in case of error. > When replacing the call to mmap() by rte_mem_map(), > the error code became NULL, breaking the API. > This function is probably not used outside of DPDK, > but it is still a problem for two reasons: > - the deprecation process was not followed > - the Linux function pci_vfio_mmap_bar() is broken for i40e > > The error code is reverted to the Unix value MAP_FAILED. > Windows needs to define this special value (-1 as in Unix). > After proper deprecation process, the API could be changed again > if really needed. > > Because of the switch from mmap() to rte_mem_map(), > another part of the API was changed: "int additional_flags" > are defined as "additional flags for the mapping range" > without mentioning it was directly used in mmap(). > Currently it is directly used in rte_mem_map(), > that's why the values rte_map_flags must be mapped (sic) on the mmap ones > in case of Unix OS. > > These are side effects of a badly defined API using Unix values. > > Bugzilla ID: 503 > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions") > Cc: talshn@mellanox.com > > Reported-by: David Marchand <david.marchand@redhat.com> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net> > --- <snip> > /* unmap addrs correctly mapped */ > for (j = 0; j < i; j++) > pci_unmap_resource( > diff --git a/lib/librte_eal/include/rte_eal_paging.h b/lib/librte_eal/include/rte_eal_paging.h > index ed98e70e9e..680a7f2505 100644 > --- a/lib/librte_eal/include/rte_eal_paging.h > +++ b/lib/librte_eal/include/rte_eal_paging.h > @@ -3,6 +3,7 @@ > */ > > #include <stdint.h> > +#include <sys/mman.h> > > #include <rte_compat.h> > > @@ -22,6 +23,7 @@ enum rte_mem_prot { > > /** Additional flags for memory mapping. */ > enum rte_map_flags { > +#ifdef RTE_EXEC_ENV_WINDOWS > /** Changes to the mapped memory are visible to other processes. */ > RTE_MAP_SHARED = 1 << 0, > /** Mapping is not backed by a regular file. */ > @@ -35,6 +37,12 @@ enum rte_map_flags { > * it is not required to do so, thus mapping with this flag may fail. > */ > RTE_MAP_FORCE_ADDRESS = 1 << 3 > +#else /* map mmap flags because they are exposed in pci_map_resource() API */ > + RTE_MAP_SHARED = MAP_SHARED, > + RTE_MAP_ANONYMOUS = MAP_ANONYMOUS, > + RTE_MAP_PRIVATE = MAP_PRIVATE, > + RTE_MAP_FORCE_ADDRESS = MAP_FIXED, > +#endif I'm probably missing something, but why is this needed? Doesn't rte_mem_map() automatically translate these flags into proper ones? pci_map_resource() will call rte_mem_map(), and that will translate these flags into their Unix equivalents. -- Thanks, Anatoly
next prev parent reply other threads:[~2020-07-10 15:39 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-10 11:53 Thomas Monjalon 2020-07-10 13:34 ` David Marchand 2020-07-10 15:39 ` Burakov, Anatoly [this message] 2020-07-10 16:17 ` Thomas Monjalon 2020-07-13 8:56 ` Burakov, Anatoly 2020-07-15 8:01 ` David Marchand 2020-07-10 17:10 ` Thomas Monjalon 2020-07-10 18:31 ` Dmitry Kozlyuk 2020-07-10 20:02 ` Thomas Monjalon 2020-07-10 20:40 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon 2020-07-10 21:07 ` Dmitry Kozlyuk 2020-07-11 9:51 ` Thomas Monjalon 2020-07-11 3:27 ` Ma, LihongX 2020-07-11 9:50 ` Thomas Monjalon 2020-07-11 3:18 ` [dpdk-dev] [PATCH] " Ma, LihongX
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3151e427-77af-80c0-e53b-4e107bb1a40c@intel.com \ --to=anatoly.burakov@intel.com \ --cc=alvinx.zhang@intel.com \ --cc=beilei.xing@intel.com \ --cc=bruce.richardson@intel.com \ --cc=david.marchand@redhat.com \ --cc=dev@dpdk.org \ --cc=dmitry.kozliuk@gmail.com \ --cc=dmitrym@microsoft.com \ --cc=ferruh.yigit@intel.com \ --cc=grive@u256.net \ --cc=jia.guo@intel.com \ --cc=navasile@linux.microsoft.com \ --cc=pallavi.kadam@intel.com \ --cc=talshn@mellanox.com \ --cc=thomas@monjalon.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git