* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-20 19:34 UTC (permalink / raw)
To: Dan Williams
Cc: Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Christoph Hellwig, Bjorn Helgaas,
Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <CAPcyv4ijztOK1FUjLuFing7ps4LOHt=6z=eO=98HHWauHA+yog@mail.gmail.com>
On 2019-06-20 12:45 p.m., Dan Williams wrote:
> On Thu, Jun 20, 2019 at 9:13 AM Logan Gunthorpe <logang@deltatee.com> wrote:
>>
>> For eons there has been a debate over whether or not to use
>> struct pages for peer-to-peer DMA transactions. Pro-pagers have
>> argued that struct pages are necessary for interacting with
>> existing code like scatterlists or the bio_vecs. Anti-pagers
>> assert that the tracking of the memory is unecessary and
>> allocating the pages is a waste of memory. Both viewpoints are
>> valid, however developers working on GPUs and RDMA tend to be
>> able to do away with struct pages relatively easily
>
> Presumably because they have historically never tried to be
> inter-operable with the block layer or drivers outside graphics and
> RDMA.
Yes, but really there are three main sets of users for P2P right now:
graphics, RDMA and NVMe. And every time a patch set comes from GPU/RDMA
people they don't bother with struct page. I seem to be the only one
trying to push P2P with NVMe and it seems to be a losing battle.
> Please spell out the value, it is not immediately obvious to me
> outside of some memory capacity savings.
There are a few things:
* Have consistency with P2P efforts as most other efforts have been
avoiding struct page. Nobody else seems to want
pci_p2pdma_add_resource() or any devm_memremap_pages() call.
* Avoid all arch-specific dependencies for P2P. With struct page the IO
memory must fit in the linear mapping. This requires some work with
RISC-V and I remember some complaints from the powerpc people regarding
this. Certainly not all arches will be able to fit the IO region into
the linear mapping space.
* Remove a bunch of PCI P2PDMA special case mapping stuff from the block
layer and RDMA interface (which I've been hearing complaints over).
* Save the struct page memory that is largely unused (as you note).
>> Previously, there have been multiple attempts[1][2] to replace
>> struct page usage with pfn_t but this has been unpopular seeing
>> it creates dangerous edge cases where unsuspecting code might
>> run accross pfn_t's they are not ready for.
>
> That's not the conclusion I arrived at because pfn_t is specifically
> an opaque type precisely to force "unsuspecting" code to throw
> compiler assertions. Instead pfn_t was dealt its death blow here:
>
> https://lore.kernel.org/lkml/CA+55aFzON9617c2_Amep0ngLq91kfrPiSccdZakxir82iekUiA@mail.gmail.com/
Ok, well yes the special pages are what we've done for P2PDMA today. But
I don't think Linus's criticism really applies to what's in this RFC.
For starters, P2PDMA doesn't, and has have never, used struct page to
look up the reference count. PCI BARs have no relation to the cache so
there's no need to serialize their access but this can be done
before/after the DMA addresses are submitted to the block/rdma layer if
it was required.
In fact, the only thing the struct page is used for in the current
P2PDMA implementation is a single flag indicating it's special and needs
to be mapped in a special way.
> My primary concern with this is that ascribes a level of generality
> that just isn't there for peer-to-peer dma operations. "Peer"
> addresses are not "DMA" addresses, and the rules about what can and
> can't do peer-DMA are not generically known to the block layer.
Correct, but I don't think we should teach the block layer about these
rules. In the current code, the rules are enforced outside the block
layer before the bios are submitted and this patch set doesn't change
that. The driver orchestrating P2P will always have to check the rules
and derive addresses from them (as appropriate). With the RFC the block
layer then doesn't have to care and can just handle the DMA addresses
directly.
> At least with a side object there's a chance to describe / recall those
> restrictions as these things get passed around the I/O stack, but an
> undecorated "DMA" address passed through the block layer with no other
> benefit to any subsystem besides RDMA does not feel like it advances
> the state of the art.
>
> Again, what are the benefits of plumbing this RDMA special case?
Because I don't think it is an RDMA special case.
Logan
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Dan Williams @ 2019-06-20 20:18 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Logan Gunthorpe, Linux Kernel Mailing List, linux-block,
linux-nvme, linux-pci, linux-rdma, Jens Axboe, Christoph Hellwig,
Bjorn Helgaas, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190620193353.GF19891@ziepe.ca>
On Thu, Jun 20, 2019 at 12:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Jun 20, 2019 at 11:45:38AM -0700, Dan Williams wrote:
>
> > > Previously, there have been multiple attempts[1][2] to replace
> > > struct page usage with pfn_t but this has been unpopular seeing
> > > it creates dangerous edge cases where unsuspecting code might
> > > run accross pfn_t's they are not ready for.
> >
> > That's not the conclusion I arrived at because pfn_t is specifically
> > an opaque type precisely to force "unsuspecting" code to throw
> > compiler assertions. Instead pfn_t was dealt its death blow here:
> >
> > https://lore.kernel.org/lkml/CA+55aFzON9617c2_Amep0ngLq91kfrPiSccdZakxir82iekUiA@mail.gmail.com/
> >
> > ...and I think that feedback also reads on this proposal.
>
> I read through Linus's remarks and it he seems completely right that
> anything that touches a filesystem needs a struct page, because FS's
> rely heavily on that.
>
> It is much less clear to me why a GPU BAR or a NVME CMB that never
> touches a filesystem needs a struct page.. The best reason I've seen
> is that it must have struct page because the block layer heavily
> depends on struct page.
>
> Since that thread was so DAX/pmem centric (and Linus did say he liked
> the __pfn_t), maybe it is worth checking again, but not for DAX/pmem
> users?
>
> This P2P is quite distinct from DAX as the struct page* would point to
> non-cacheable weird memory that few struct page users would even be
> able to work with, while I understand DAX use cases focused on CPU
> cache coherent memory, and filesystem involvement.
What I'm poking at is whether this block layer capability can pick up
users outside of RDMA, more on this below...
>
> > My primary concern with this is that ascribes a level of generality
> > that just isn't there for peer-to-peer dma operations. "Peer"
> > addresses are not "DMA" addresses, and the rules about what can and
> > can't do peer-DMA are not generically known to the block layer.
>
> ?? The P2P infrastructure produces a DMA bus address for the
> initiating device that is is absolutely a DMA address. There is some
> intermediate CPU centric representation, but after mapping it is the
> same as any other DMA bus address.
Right, this goes back to the confusion caused by the hardware / bus /
address that a dma-engine would consume directly, and Linux "DMA"
address as a device-specific translation of host memory.
Is the block layer representation of this address going to go through
a peer / "bus" address translation when it reaches the RDMA driver? In
other words if we tried to use this facility with other drivers how
would the driver know it was passed a traditional Linux DMA address,
vs a peer bus address that the device may not be able to handle?
> The map function can tell if the device pair combination can do p2p or
> not.
Ok, if this map step is still there then reduce a significant portion
of my concern and it becomes a quibble about the naming and how a
non-RDMA device driver might figure out if it was handled an address
it can't handle.
>
> > Again, what are the benefits of plumbing this RDMA special case?
>
> It is not just RDMA, this is interesting for GPU and vfio use cases
> too. RDMA is just the most complete in-tree user we have today.
>
> ie GPU people wouuld really like to do read() and have P2P
> transparently happen to on-GPU pages. With GPUs having huge amounts of
> memory loading file data into them is really a performance critical
> thing.
A direct-i/o read(2) into a page-less GPU mapping? Through a regular
file or a device special file?
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-20 20:51 UTC (permalink / raw)
To: Dan Williams, Jason Gunthorpe
Cc: Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Christoph Hellwig, Bjorn Helgaas,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <CAPcyv4jyNRBvtWhr9+aHbzWP6=D4qAME+=hWMtOYJ17BVHdy2w@mail.gmail.com>
On 2019-06-20 2:18 p.m., Dan Williams wrote:
>> Since that thread was so DAX/pmem centric (and Linus did say he liked
>> the __pfn_t), maybe it is worth checking again, but not for DAX/pmem
>> users?
>>
>> This P2P is quite distinct from DAX as the struct page* would point to
>> non-cacheable weird memory that few struct page users would even be
>> able to work with, while I understand DAX use cases focused on CPU
>> cache coherent memory, and filesystem involvement.
>
> What I'm poking at is whether this block layer capability can pick up
> users outside of RDMA, more on this below...
I assume you mean outside of P2PDMA....
This new block layer capability is more likely to pick up additional
users compared to the existing block layer changes that are *very*
specific to PCI P2PDMA.
I also have (probably significantly controversial) plans to use this to
allow P2P through user space with O_DIRECT using an idea Jerome had in a
previous patch set that was discussed a bit informally at LSF/MM this
year. But that's a whole other RFC and requires a bunch of work I
haven't done yet.
>>
>>> My primary concern with this is that ascribes a level of generality
>>> that just isn't there for peer-to-peer dma operations. "Peer"
>>> addresses are not "DMA" addresses, and the rules about what can and
>>> can't do peer-DMA are not generically known to the block layer.
>>
>> ?? The P2P infrastructure produces a DMA bus address for the
>> initiating device that is is absolutely a DMA address. There is some
>> intermediate CPU centric representation, but after mapping it is the
>> same as any other DMA bus address.
>
> Right, this goes back to the confusion caused by the hardware / bus /
> address that a dma-engine would consume directly, and Linux "DMA"
> address as a device-specific translation of host memory.
>
> Is the block layer representation of this address going to go through
> a peer / "bus" address translation when it reaches the RDMA driver? In
> other words if we tried to use this facility with other drivers how
> would the driver know it was passed a traditional Linux DMA address,
> vs a peer bus address that the device may not be able to handle?
The idea is that the driver doesn't need to know. There's no distinction
between a Linux DMA address and a peer bus address. They are both used
for the same purpose: to program into a DMA engine. If the device cannot
handle such a DMA address then it shouldn't indicate support for this
feature or the P2PDMA layer needs a way to detect this. Really, this
property depends more on the bus than the device and that's what all the
P2PDMA code in the PCI tree handles.
>> The map function can tell if the device pair combination can do p2p or
>> not.
>
> Ok, if this map step is still there then reduce a significant portion
> of my concern and it becomes a quibble about the naming and how a
> non-RDMA device driver might figure out if it was handled an address
> it can't handle.
Yes, there will always be a map step, but it should be done by the
orchestrator because it requires both devices (the client and the
provider) and the block layer really should not know about both devices.
In this RFC, the map step is kind of hidden but would probably come back
in the future. It's currently a call to pci_p2pmem_virt_to_bus() but
would eventually need to be a pci_p2pmem_map_resource() or similar which
takes a pointer to the pci_dev provider and the struct device client
doing the mapping.
Logan
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Dan Williams @ 2019-06-20 23:40 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Christoph Hellwig, Bjorn Helgaas,
Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <91eba9a0-27b4-08b4-7c12-86e24e1bfe85@deltatee.com>
On Thu, Jun 20, 2019 at 12:35 PM Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
>
> On 2019-06-20 12:45 p.m., Dan Williams wrote:
> > On Thu, Jun 20, 2019 at 9:13 AM Logan Gunthorpe <logang@deltatee.com> wrote:
> >>
> >> For eons there has been a debate over whether or not to use
> >> struct pages for peer-to-peer DMA transactions. Pro-pagers have
> >> argued that struct pages are necessary for interacting with
> >> existing code like scatterlists or the bio_vecs. Anti-pagers
> >> assert that the tracking of the memory is unecessary and
> >> allocating the pages is a waste of memory. Both viewpoints are
> >> valid, however developers working on GPUs and RDMA tend to be
> >> able to do away with struct pages relatively easily
> >
> > Presumably because they have historically never tried to be
> > inter-operable with the block layer or drivers outside graphics and
> > RDMA.
>
> Yes, but really there are three main sets of users for P2P right now:
> graphics, RDMA and NVMe. And every time a patch set comes from GPU/RDMA
> people they don't bother with struct page. I seem to be the only one
> trying to push P2P with NVMe and it seems to be a losing battle.
>
> > Please spell out the value, it is not immediately obvious to me
> > outside of some memory capacity savings.
>
> There are a few things:
>
> * Have consistency with P2P efforts as most other efforts have been
> avoiding struct page. Nobody else seems to want
> pci_p2pdma_add_resource() or any devm_memremap_pages() call.
>
> * Avoid all arch-specific dependencies for P2P. With struct page the IO
> memory must fit in the linear mapping. This requires some work with
> RISC-V and I remember some complaints from the powerpc people regarding
> this. Certainly not all arches will be able to fit the IO region into
> the linear mapping space.
>
> * Remove a bunch of PCI P2PDMA special case mapping stuff from the block
> layer and RDMA interface (which I've been hearing complaints over).
This seems to be the most salient point. I was missing the fact that
this replaces custom hacks and "special" pages with an explicit "just
pass this pre-mapped address down the stack". It's functionality that
might plausibly be used outside of p2p, as long as the driver can
assert that it never needs to touch the data with the cpu before
handing it off to a dma-engine.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-20 23:42 UTC (permalink / raw)
To: Dan Williams
Cc: Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Christoph Hellwig, Bjorn Helgaas,
Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <CAPcyv4gPOXaL3qks6RMufu==O9RV2m_-7bBmJqKOFYTf4v_jXQ@mail.gmail.com>
On 2019-06-20 5:40 p.m., Dan Williams wrote:
> This seems to be the most salient point. I was missing the fact that
> this replaces custom hacks and "special" pages with an explicit "just
> pass this pre-mapped address down the stack". It's functionality that
> might plausibly be used outside of p2p, as long as the driver can
> assert that it never needs to touch the data with the cpu before
> handing it off to a dma-engine.
Yup, that's a good way to put it. If I resend this patchset, I'll
include wording like yours in the cover letter.
Logan
^ permalink raw reply
* Re: [PATCH iproute2-next v2 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: Denis Kirjanov @ 2019-06-21 10:30 UTC (permalink / raw)
To: stephen; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
In-Reply-To: <20190620090249.106704-1-dkirjanov@suse.com>
On 6/20/19, Denis Kirjanov <kda@linux-powerpc.org> wrote:
> Current code assumes that we print Etheret mac and
> that doesn't work in IPoIB case with SRIOV-enabled hardware
>
> Before:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
> trust off, query_rss off
> ...
>
> After:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
> checking off, link-state disable, trust off, query_rss off
>
> v1->v2: updated kernel headers to uapi commit
>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
Hi Stephen,
are you going to take the patches?
Thanks!
> ---
> ip/ipaddress.c | 42 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index b504200b..13ad76dd 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -26,6 +26,7 @@
>
> #include <linux/netdevice.h>
> #include <linux/if_arp.h>
> +#include <linux/if_infiniband.h>
> #include <linux/sockios.h>
> #include <linux/net_namespace.h>
>
> @@ -349,9 +350,10 @@ static void print_af_spec(FILE *fp, struct rtattr
> *af_spec_attr)
>
> static void print_vf_stats64(FILE *fp, struct rtattr *vfstats);
>
> -static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> +static void print_vfinfo(struct ifinfomsg *ifi, FILE *fp, struct rtattr
> *vfinfo)
> {
> struct ifla_vf_mac *vf_mac;
> + struct ifla_vf_broadcast *vf_broadcast;
> struct ifla_vf_tx_rate *vf_tx_rate;
> struct rtattr *vf[IFLA_VF_MAX + 1] = {};
>
> @@ -365,13 +367,43 @@ static void print_vfinfo(FILE *fp, struct rtattr
> *vfinfo)
> parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
>
> vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
> + vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
> vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>
> print_string(PRINT_FP, NULL, "%s ", _SL_);
> print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
> - print_string(PRINT_ANY, "mac", "MAC %s",
> - ll_addr_n2a((unsigned char *) &vf_mac->mac,
> - ETH_ALEN, 0, b1, sizeof(b1)));
> +
> + print_string(PRINT_ANY,
> + "link_type",
> + " link/%s ",
> + ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
> +
> + print_color_string(PRINT_ANY,
> + COLOR_MAC,
> + "address",
> + "%s",
> + ll_addr_n2a((unsigned char *) &vf_mac->mac,
> + ifi->ifi_type == ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
> + ifi->ifi_type,
> + b1, sizeof(b1)));
> +
> + if (vf[IFLA_VF_BROADCAST]) {
> + if (ifi->ifi_flags&IFF_POINTOPOINT) {
> + print_string(PRINT_FP, NULL, " peer ", NULL);
> + print_bool(PRINT_JSON,
> + "link_pointtopoint", NULL, true);
> + } else {
> + print_string(PRINT_FP, NULL, " brd ", NULL);
> + }
> + print_color_string(PRINT_ANY,
> + COLOR_MAC,
> + "broadcast",
> + "%s",
> + ll_addr_n2a((unsigned char *)
> &vf_broadcast->broadcast,
> + ifi->ifi_type ==
> ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
> + ifi->ifi_type,
> + b1, sizeof(b1)));
> + }
>
> if (vf[IFLA_VF_VLAN_LIST]) {
> struct rtattr *i, *vfvlanlist = vf[IFLA_VF_VLAN_LIST];
> @@ -1102,7 +1134,7 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
> open_json_array(PRINT_JSON, "vfinfo_list");
> for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
> open_json_object(NULL);
> - print_vfinfo(fp, i);
> + print_vfinfo(ifi, fp, i);
> close_json_object();
> }
> close_json_array(PRINT_JSON, NULL);
> --
> 2.12.3
>
>
^ permalink raw reply
* [PATCH] RDMA/core: Fix race when resolving IP address
From: Dag Moxnes @ 2019-06-21 14:09 UTC (permalink / raw)
To: dag.moxnes, dledford, jgg, leon; +Cc: linux-rdma, linux-kernel
Use neighbour lock when copying MAC address from neighbour data struct
in dst_fetch_ha.
When not using the lock, it is possible for the function to race with
neigh_update, causing it to copy an invalid MAC address.
It is possible to provoke this error by calling rdma_resolve_addr in a
tight loop, while deleting the corresponding ARP entry in another tight
loop.
Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com>
Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4
---
drivers/infiniband/core/addr.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 2f7d141598..e4945fd1bb 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst,
if (!n)
return -ENODATA;
+ read_lock_bh(&n->lock)
if (!(n->nud_state & NUD_VALID)) {
- neigh_event_send(n, NULL);
ret = -ENODATA;
} else {
memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN);
}
+ read_unlock_bh(&n->lock);
+
+ if (ret)
+ neigh_event_send(n, NULL);
neigh_release(n);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] RDMA/core: Fix race when resolving IP address
From: Jason Gunthorpe @ 2019-06-21 14:56 UTC (permalink / raw)
To: Dag Moxnes; +Cc: dledford, leon, linux-rdma, linux-kernel, Parav Pandit
In-Reply-To: <1561126156-10162-1-git-send-email-dag.moxnes@oracle.com>
On Fri, Jun 21, 2019 at 04:09:16PM +0200, Dag Moxnes wrote:
> Use neighbour lock when copying MAC address from neighbour data struct
> in dst_fetch_ha.
>
> When not using the lock, it is possible for the function to race with
> neigh_update, causing it to copy an invalid MAC address.
>
> It is possible to provoke this error by calling rdma_resolve_addr in a
> tight loop, while deleting the corresponding ARP entry in another tight
> loop.
>
> Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com>
> Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4
> drivers/infiniband/core/addr.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> index 2f7d141598..e4945fd1bb 100644
> +++ b/drivers/infiniband/core/addr.c
> @@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst,
> if (!n)
> return -ENODATA;
>
> + read_lock_bh(&n->lock)
> if (!(n->nud_state & NUD_VALID)) {
> - neigh_event_send(n, NULL);
> ret = -ENODATA;
> } else {
> memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN);
> }
> + read_unlock_bh(&n->lock);
> +
> + if (ret)
> + neigh_event_send(n, NULL);
>
> neigh_release(n);
Can we write this with less spaghetti please, maybe:
static int dst_fetch_ha(const struct dst_entry *dst,
struct rdma_dev_addr *dev_addr,
const void *daddr)
{
struct neighbour *n;
int ret = 0;
n = dst_neigh_lookup(dst, daddr);
if (!n)
return -ENODATA;
read_lock_bh(&n->lock);
if (!(n->nud_state & NUD_VALID)) {
read_unlock_bh(&n->lock);
goto out_send;
}
memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN);
read_unlock_bh(&n->lock);
goto out_release;
out_send:
neigh_event_send(n, NULL);
ret = -ENODATA;
out_release:
neigh_release(n);
return ret;
}
Also, Parav should look at it.
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Doug Ledford @ 2019-06-21 15:49 UTC (permalink / raw)
To: Michal Kalderon, ariel.elior, jgg; +Cc: linux-rdma, davem, netdev
In-Reply-To: <20190613083819.6998-1-michal.kalderon@marvell.com>
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]
On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> This patch series used the doorbell overflow recovery mechanism
> introduced in
> commit 36907cd5cd72 ("qed: Add doorbell overflow recovery mechanism")
> for rdma ( RoCE and iWARP )
>
> rdma-core pull request #493
>
> Changes from V2:
> - Don't use long-lived kmap. Instead use user-trigger mmap for the
> doorbell recovery entries.
> - Modify dpi_addr to be denoted with __iomem and avoid redundant
> casts
>
> Changes from V1:
> - call kmap to map virtual address into kernel space
> - modify db_rec_delete to be void
> - remove some cpu_to_le16 that were added to previous patch which are
> correct but not related to the overflow recovery mechanism. Will be
> submitted as part of a different patch
>
>
> Michal Kalderon (3):
> qed*: Change dpi_addr to be denoted with __iomem
> RDMA/qedr: Add doorbell overflow recovery support
> RDMA/qedr: Add iWARP doorbell recovery support
>
> drivers/infiniband/hw/qedr/main.c | 2 +-
> drivers/infiniband/hw/qedr/qedr.h | 27 +-
> drivers/infiniband/hw/qedr/verbs.c | 387
> ++++++++++++++++++++++++-----
> drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> include/linux/qed/qed_rdma_if.h | 2 +-
> include/uapi/rdma/qedr-abi.h | 25 ++
> 6 files changed, 378 insertions(+), 71 deletions(-)
>
Hi Michal,
In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem
*). Why not just define the struct elements as u8 __iomem * instead of
void __iomem * and avoid all the casts?
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Jason Gunthorpe @ 2019-06-21 17:47 UTC (permalink / raw)
To: Dan Williams
Cc: Logan Gunthorpe, Linux Kernel Mailing List, linux-block,
linux-nvme, linux-pci, linux-rdma, Jens Axboe, Christoph Hellwig,
Bjorn Helgaas, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <CAPcyv4jyNRBvtWhr9+aHbzWP6=D4qAME+=hWMtOYJ17BVHdy2w@mail.gmail.com>
On Thu, Jun 20, 2019 at 01:18:13PM -0700, Dan Williams wrote:
> > This P2P is quite distinct from DAX as the struct page* would point to
> > non-cacheable weird memory that few struct page users would even be
> > able to work with, while I understand DAX use cases focused on CPU
> > cache coherent memory, and filesystem involvement.
>
> What I'm poking at is whether this block layer capability can pick up
> users outside of RDMA, more on this below...
The generic capability is to do a transfer through the block layer and
scatter/gather the resulting data to some PCIe BAR memory. Currently
the block layer can only scatter/gather data into CPU cache coherent
memory.
We know of several useful places to put PCIe BAR memory already:
- On a GPU (or FPGA, acclerator, etc), ie the GB's of GPU private
memory that is standard these days.
- On a NVMe CMB. This lets the NVMe drive avoid DMA entirely
- On a RDMA NIC. Mellanox NICs have a small amount of BAR memory that
can be used like a CMB and avoids a DMA
RDMA doesn't really get so involved here, except that RDMA is often
the prefered way to source/sink the data buffers after the block layer has
scatter/gathered to them. (and of course RDMA is often for a block
driver, ie NMVe over fabrics)
> > > My primary concern with this is that ascribes a level of generality
> > > that just isn't there for peer-to-peer dma operations. "Peer"
> > > addresses are not "DMA" addresses, and the rules about what can and
> > > can't do peer-DMA are not generically known to the block layer.
> >
> > ?? The P2P infrastructure produces a DMA bus address for the
> > initiating device that is is absolutely a DMA address. There is some
> > intermediate CPU centric representation, but after mapping it is the
> > same as any other DMA bus address.
>
> Right, this goes back to the confusion caused by the hardware / bus /
> address that a dma-engine would consume directly, and Linux "DMA"
> address as a device-specific translation of host memory.
I don't think there is a confusion :) Logan explained it, the
dma_addr_t is always the thing you program into the DMA engine of the
device it was created for, and this changes nothing about that.
Think of the dma vec as the same as a dma mapped SGL, just with no
available struct page.
> Is the block layer representation of this address going to go through
> a peer / "bus" address translation when it reaches the RDMA driver?
No, it is just like any other dma mapped SGL, it is ready to go for
the device it was mapped for, and can be used for nothing other than
programming DMA on that device.
> > ie GPU people wouuld really like to do read() and have P2P
> > transparently happen to on-GPU pages. With GPUs having huge amounts of
> > memory loading file data into them is really a performance critical
> > thing.
>
> A direct-i/o read(2) into a page-less GPU mapping?
The interesting case is probably an O_DIRECT read into a
DEVICE_PRIVATE page owned by the GPU driver and mmaped into the
process calling read(). The GPU driver can dynamically arrange for
that DEVICE_PRIVATE page to linked to P2P targettable BAR memory so
the HW is capable of a direct CPU bypass transfer from the underlying
block device (ie NVMe or RDMA) to the GPU.
One way to approach this problem is to use this new dma_addr path in
the block layer.
Another way is to feed the DEVICE_PRIVATE pages into the block layer
and have it DMA map them to a P2P address.
In either case we have a situation where the block layer cannot touch
the target struct page buffers with the CPU because there is no cache
coherent CPU mapping for them, and we have to create a CPU clean path
in the block layer.
At best you could do memcpy to/from on these things, but if a GPU is
involved even that is incredibly inefficient. The GPU can do the
memcpy with DMA much faster than a memcpy_to/from_io.
Jason
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Dan Williams @ 2019-06-21 17:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Logan Gunthorpe, Linux Kernel Mailing List, linux-block,
linux-nvme, linux-pci, linux-rdma, Jens Axboe, Christoph Hellwig,
Bjorn Helgaas, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190621174724.GV19891@ziepe.ca>
On Fri, Jun 21, 2019 at 10:47 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Jun 20, 2019 at 01:18:13PM -0700, Dan Williams wrote:
>
> > > This P2P is quite distinct from DAX as the struct page* would point to
> > > non-cacheable weird memory that few struct page users would even be
> > > able to work with, while I understand DAX use cases focused on CPU
> > > cache coherent memory, and filesystem involvement.
> >
> > What I'm poking at is whether this block layer capability can pick up
> > users outside of RDMA, more on this below...
>
> The generic capability is to do a transfer through the block layer and
> scatter/gather the resulting data to some PCIe BAR memory. Currently
> the block layer can only scatter/gather data into CPU cache coherent
> memory.
>
> We know of several useful places to put PCIe BAR memory already:
> - On a GPU (or FPGA, acclerator, etc), ie the GB's of GPU private
> memory that is standard these days.
> - On a NVMe CMB. This lets the NVMe drive avoid DMA entirely
> - On a RDMA NIC. Mellanox NICs have a small amount of BAR memory that
> can be used like a CMB and avoids a DMA
>
> RDMA doesn't really get so involved here, except that RDMA is often
> the prefered way to source/sink the data buffers after the block layer has
> scatter/gathered to them. (and of course RDMA is often for a block
> driver, ie NMVe over fabrics)
>
> > > > My primary concern with this is that ascribes a level of generality
> > > > that just isn't there for peer-to-peer dma operations. "Peer"
> > > > addresses are not "DMA" addresses, and the rules about what can and
> > > > can't do peer-DMA are not generically known to the block layer.
> > >
> > > ?? The P2P infrastructure produces a DMA bus address for the
> > > initiating device that is is absolutely a DMA address. There is some
> > > intermediate CPU centric representation, but after mapping it is the
> > > same as any other DMA bus address.
> >
> > Right, this goes back to the confusion caused by the hardware / bus /
> > address that a dma-engine would consume directly, and Linux "DMA"
> > address as a device-specific translation of host memory.
>
> I don't think there is a confusion :) Logan explained it, the
> dma_addr_t is always the thing you program into the DMA engine of the
> device it was created for, and this changes nothing about that.
Yup, Logan and I already settled that point on our last exchange and
offered to make that clearer in the changelog.
^ permalink raw reply
* [PULL REQUEST] Please pull rdma.git
From: Doug Ledford @ 2019-06-21 19:42 UTC (permalink / raw)
To: Torvalds, Linus; +Cc: Gunthorpe, Jason, linux-rdma, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3424 bytes --]
Hi Linus,
This is probably our last -rc pull request. We don't have anything else
outstanding at the moment anyway, and with the summer months on us and
people taking trips, I expect the next weeks leading up to the merge
window to be pretty calm and sedate.
This has two simple, no brainer fixes for the EFA driver.
Then it has ten not quite so simple fixes for the hfi1 driver. The
problem with them is that they aren't simply one liner typo fixes.
They're still fixes, but they're more complex issues like livelock under
heavy load where the answer was to change work queue usage and spinlock
usage to resolve the problem, or issues with orphaned requests during
certain types of failures like link down which required some more
complex work to fix too. They all look like legitimate fixes to me,
they just aren't small like I wish they were.
Here's the boilerplate:
The following changes since commit d1fdb6d8f6a4109a4263176c84b899076a5f8008:
Linux 5.2-rc4 (2019-06-08 20:24:46 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus
for you to fetch changes up to 7a5834e456f7fb3eca9b63af2a6bc7f460ae482f:
RDMA/efa: Handle mmap insertions overflow (2019-06-18 16:27:24 -0400)
----------------------------------------------------------------
Pull request for 5.1-rc5
- 2 minor EFA fixes
- 10 hfi1 fixes related to scaling issues
----------------------------------------------------------------
Gal Pressman (2):
RDMA/efa: Fix success return value in case of error
RDMA/efa: Handle mmap insertions overflow
Kaike Wan (1):
IB/hfi1: Validate fault injection opcode user input
Mike Marciniszyn (9):
IB/hfi1: Close PSM sdma_progress sleep window
IB/hfi1: Correct tid qp rcd to match verbs context
IB/hfi1: Avoid hardlockup with flushlist_lock
IB/hfi1: Silence txreq allocation warnings
IB/hfi1: Create inline to get extended headers
IB/hfi1: Use aborts to trigger RC throttling
IB/hfi1: Wakeup QPs orphaned on wait list after flush
IB/hfi1: Handle wakeup of orphaned QPs for pio
IB/hfi1: Handle port down properly in pio
drivers/infiniband/hw/efa/efa_com_cmd.c | 24 +++++++++++----
drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++---
drivers/infiniband/hw/hfi1/chip.c | 13 ++++++++
drivers/infiniband/hw/hfi1/chip.h | 1 +
drivers/infiniband/hw/hfi1/fault.c | 5 +++
drivers/infiniband/hw/hfi1/hfi.h | 31 +++++++++++++++++++
drivers/infiniband/hw/hfi1/pio.c | 21 +++++++++++--
drivers/infiniband/hw/hfi1/rc.c | 53 +++++++++++++++++++-------------
drivers/infiniband/hw/hfi1/sdma.c | 26 ++++++++++++----
drivers/infiniband/hw/hfi1/tid_rdma.c | 4 +--
drivers/infiniband/hw/hfi1/ud.c | 4 +--
drivers/infiniband/hw/hfi1/user_sdma.c | 12 +++-----
drivers/infiniband/hw/hfi1/user_sdma.h | 1 -
drivers/infiniband/hw/hfi1/verbs.c | 14 +++++----
drivers/infiniband/hw/hfi1/verbs.h | 1 +
drivers/infiniband/hw/hfi1/verbs_txreq.c | 2 +-
drivers/infiniband/hw/hfi1/verbs_txreq.h | 3 +-
17 files changed, 174 insertions(+), 62 deletions(-)
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Michal Kalderon @ 2019-06-21 19:49 UTC (permalink / raw)
To: Doug Ledford, Ariel Elior, jgg@ziepe.ca
Cc: linux-rdma@vger.kernel.org, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <bda0321cb362bc93f5428b1df7daf69fed083656.camel@redhat.com>
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Doug Ledford
>
> On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> > This patch series used the doorbell overflow recovery mechanism
> > introduced in commit 36907cd5cd72 ("qed: Add doorbell overflow
> > recovery mechanism") for rdma ( RoCE and iWARP )
> >
> > rdma-core pull request #493
> >
> > Changes from V2:
> > - Don't use long-lived kmap. Instead use user-trigger mmap for the
> > doorbell recovery entries.
> > - Modify dpi_addr to be denoted with __iomem and avoid redundant
> > casts
> >
> > Changes from V1:
> > - call kmap to map virtual address into kernel space
> > - modify db_rec_delete to be void
> > - remove some cpu_to_le16 that were added to previous patch which are
> > correct but not related to the overflow recovery mechanism. Will be
> > submitted as part of a different patch
> >
> >
> > Michal Kalderon (3):
> > qed*: Change dpi_addr to be denoted with __iomem
> > RDMA/qedr: Add doorbell overflow recovery support
> > RDMA/qedr: Add iWARP doorbell recovery support
> >
> > drivers/infiniband/hw/qedr/main.c | 2 +-
> > drivers/infiniband/hw/qedr/qedr.h | 27 +-
> > drivers/infiniband/hw/qedr/verbs.c | 387
> > ++++++++++++++++++++++++-----
> > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> > include/linux/qed/qed_rdma_if.h | 2 +-
> > include/uapi/rdma/qedr-abi.h | 25 ++
> > 6 files changed, 378 insertions(+), 71 deletions(-)
> >
>
> Hi Michal,
>
> In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem *).
> Why not just define the struct elements as u8 __iomem * instead of void
> __iomem * and avoid all the casts?
>
Hi Doug,
Thanks for the review. The remaining casts are due to pointer arithmetic and not variable assignments
as before. Removing the cast entirely will require quite a lot of changes in qed and in rdma-core
which I would be happy to avoid at this time.
Please reconsider,
Thanks again
Michal
> --
> Doug Ledford <dledford@redhat.com>
> GPG KeyID: B826A3330E572FDD
> Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply
* Re: [PATCH v3 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Jason Gunthorpe @ 2019-06-21 19:58 UTC (permalink / raw)
To: Michal Kalderon
Cc: Doug Ledford, Ariel Elior, linux-rdma@vger.kernel.org,
davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <MN2PR18MB3182498CA8C9C7EB3259F62FA1E70@MN2PR18MB3182.namprd18.prod.outlook.com>
On Fri, Jun 21, 2019 at 07:49:39PM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Doug Ledford
> >
> > On Thu, 2019-06-13 at 11:38 +0300, Michal Kalderon wrote:
> > > This patch series used the doorbell overflow recovery mechanism
> > > introduced in commit 36907cd5cd72 ("qed: Add doorbell overflow
> > > recovery mechanism") for rdma ( RoCE and iWARP )
> > >
> > > rdma-core pull request #493
> > >
> > > Changes from V2:
> > > - Don't use long-lived kmap. Instead use user-trigger mmap for the
> > > doorbell recovery entries.
> > > - Modify dpi_addr to be denoted with __iomem and avoid redundant
> > > casts
> > >
> > > Changes from V1:
> > > - call kmap to map virtual address into kernel space
> > > - modify db_rec_delete to be void
> > > - remove some cpu_to_le16 that were added to previous patch which are
> > > correct but not related to the overflow recovery mechanism. Will be
> > > submitted as part of a different patch
> > >
> > >
> > > Michal Kalderon (3):
> > > qed*: Change dpi_addr to be denoted with __iomem
> > > RDMA/qedr: Add doorbell overflow recovery support
> > > RDMA/qedr: Add iWARP doorbell recovery support
> > >
> > > drivers/infiniband/hw/qedr/main.c | 2 +-
> > > drivers/infiniband/hw/qedr/qedr.h | 27 +-
> > > drivers/infiniband/hw/qedr/verbs.c | 387
> > > ++++++++++++++++++++++++-----
> > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 6 +-
> > > include/linux/qed/qed_rdma_if.h | 2 +-
> > > include/uapi/rdma/qedr-abi.h | 25 ++
> > > 6 files changed, 378 insertions(+), 71 deletions(-)
> > >
> >
> > Hi Michal,
> >
> > In patch 2 and 3 both, you still have quite a few casts to (u8 __iomem *).
> > Why not just define the struct elements as u8 __iomem * instead of void
> > __iomem * and avoid all the casts?
> >
> Hi Doug,
>
> Thanks for the review. The remaining casts are due to pointer arithmetic and not variable assignments
> as before. Removing the cast entirely will require quite a lot of changes in qed and in rdma-core
> which I would be happy to avoid at this time.
In linux pointer math on a void * acts the same as a u8 so you should
never need to cast a void * to a u8 just to do math?
Jason
^ permalink raw reply
* Re: [PULL REQUEST] Please pull rdma.git
From: pr-tracker-bot @ 2019-06-21 22:35 UTC (permalink / raw)
To: Doug Ledford; +Cc: Torvalds, Linus, Gunthorpe, Jason, linux-rdma, linux-kernel
In-Reply-To: <3ed43fc8399c5b8efa262699a1d3559cbe41fed5.camel@redhat.com>
The pull request you sent on Fri, 21 Jun 2019 15:42:09 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/121bddf39a8e39baf0df9ef1d688392c179935cd
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH iproute2-next v2 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: David Ahern @ 2019-06-21 22:56 UTC (permalink / raw)
To: Denis Kirjanov, stephen; +Cc: netdev, linux-rdma, dledford, mkubecek
In-Reply-To: <20190620090249.106704-1-dkirjanov@suse.com>
On 6/20/19 3:02 AM, Denis Kirjanov wrote:
> @@ -349,9 +350,10 @@ static void print_af_spec(FILE *fp, struct rtattr *af_spec_attr)
>
> static void print_vf_stats64(FILE *fp, struct rtattr *vfstats);
>
> -static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> +static void print_vfinfo(struct ifinfomsg *ifi, FILE *fp, struct rtattr *vfinfo)
> {
> struct ifla_vf_mac *vf_mac;
> + struct ifla_vf_broadcast *vf_broadcast;
> struct ifla_vf_tx_rate *vf_tx_rate;
> struct rtattr *vf[IFLA_VF_MAX + 1] = {};
>
> @@ -365,13 +367,43 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
> parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
>
> vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
> + vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
> vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>
> print_string(PRINT_FP, NULL, "%s ", _SL_);
> print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
> - print_string(PRINT_ANY, "mac", "MAC %s",
> - ll_addr_n2a((unsigned char *) &vf_mac->mac,
> - ETH_ALEN, 0, b1, sizeof(b1)));
> +
> + print_string(PRINT_ANY,
> + "link_type",
> + " link/%s ",
> + ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
> +
> + print_color_string(PRINT_ANY,
> + COLOR_MAC,
> + "address",
> + "%s",
> + ll_addr_n2a((unsigned char *) &vf_mac->mac,
> + ifi->ifi_type == ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
> + ifi->ifi_type,
> + b1, sizeof(b1)));
> +
> + if (vf[IFLA_VF_BROADCAST]) {
> + if (ifi->ifi_flags&IFF_POINTOPOINT) {
> + print_string(PRINT_FP, NULL, " peer ", NULL);
> + print_bool(PRINT_JSON,
> + "link_pointtopoint", NULL, true);
> + } else {
> + print_string(PRINT_FP, NULL, " brd ", NULL);
> + }
> + print_color_string(PRINT_ANY,
> + COLOR_MAC,
> + "broadcast",
> + "%s",
> + ll_addr_n2a((unsigned char *) &vf_broadcast->broadcast,
> + ifi->ifi_type == ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
> + ifi->ifi_type,
> + b1, sizeof(b1)));
> + }
>
you have a number of alignment problems with the above changes. you can
run checkpatch from the kernel repo on it to verify the coding standards.
^ permalink raw reply
* [PATCH v3 1/2] [v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
From: Ajay Kaher @ 2019-06-22 13:02 UTC (permalink / raw)
To: aarcange, jannh, oleg, peterx, rppt, jgg, mhocko
Cc: jglisse, akpm, mike.kravetz, viro, riandrews, arve, yishaih,
dledford, sean.hefty, hal.rosenstock, matanb, leonro,
linux-fsdevel, linux-mm, devel, linux-rdma, linux-kernel, stable,
akaher, srivatsab, amakhalov, Linus Torvalds, Greg Kroah-Hartman
From: Andrea Arcangeli <aarcange@redhat.com>
commit 04f5866e41fb70690e28397487d8bd8eea7d712a upstream.
The core dumping code has always run without holding the mmap_sem for
writing, despite that is the only way to ensure that the entire vma
layout will not change from under it. Only using some signal
serialization on the processes belonging to the mm is not nearly enough.
This was pointed out earlier. For example in Hugh's post from Jul 2017:
https://lkml.kernel.org/r/alpine.LSU.2.11.1707191716030.2055@eggly.anvils
"Not strictly relevant here, but a related note: I was very surprised
to discover, only quite recently, how handle_mm_fault() may be called
without down_read(mmap_sem) - when core dumping. That seems a
misguided optimization to me, which would also be nice to correct"
In particular because the growsdown and growsup can move the
vm_start/vm_end the various loops the core dump does around the vma will
not be consistent if page faults can happen concurrently.
Pretty much all users calling mmget_not_zero()/get_task_mm() and then
taking the mmap_sem had the potential to introduce unexpected side
effects in the core dumping code.
Adding mmap_sem for writing around the ->core_dump invocation is a
viable long term fix, but it requires removing all copy user and page
faults and to replace them with get_dump_page() for all binary formats
which is not suitable as a short term fix.
For the time being this solution manually covers the places that can
confuse the core dump either by altering the vma layout or the vma flags
while it runs. Once ->core_dump runs under mmap_sem for writing the
function mmget_still_valid() can be dropped.
Allowing mmap_sem protected sections to run in parallel with the
coredump provides some minor parallelism advantage to the swapoff code
(which seems to be safe enough by never mangling any vma field and can
keep doing swapins in parallel to the core dumping) and to some other
corner case.
In order to facilitate the backporting I added "Fixes: 86039bd3b4e6"
however the side effect of this same race condition in /proc/pid/mem
should be reproducible since before 2.6.12-rc2 so I couldn't add any
other "Fixes:" because there's no hash beyond the git genesis commit.
Because find_extend_vma() is the only location outside of the process
context that could modify the "mm" structures under mmap_sem for
reading, by adding the mmget_still_valid() check to it, all other cases
that take the mmap_sem for reading don't need the new check after
mmget_not_zero()/get_task_mm(). The expand_stack() in page fault
context also doesn't need the new check, because all tasks under core
dumping are frozen.
Link: http://lkml.kernel.org/r/20190325224949.11068-1-aarcange@redhat.com
Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Reported-by: Jann Horn <jannh@google.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Jann Horn <jannh@google.com>
Acked-by: Jason Gunthorpe <jgg@mellanox.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[akaher@vmware.com: stable 4.9 backport
- handle binder_update_page_range - mhocko@suse.com]
Signed-off-by: Ajay Kaher <akaher@vmware.com>
---
drivers/android/binder.c | 6 ++++++
fs/proc/task_mmu.c | 18 ++++++++++++++++++
fs/userfaultfd.c | 9 +++++++++
include/linux/mm.h | 21 +++++++++++++++++++++
mm/mmap.c | 6 +++++-
5 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 80499f4..f05ab8f 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -581,6 +581,12 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
if (mm) {
down_write(&mm->mmap_sem);
+ if (!mmget_still_valid(mm)) {
+ if (allocate == 0)
+ goto free_range;
+ goto err_no_vma;
+ }
+
vma = proc->vma;
if (vma && mm != proc->vma_vm_mm) {
pr_err("%d: vma mm and task mm mismatch\n",
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 5138e78..4b207b1 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1057,6 +1057,24 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
count = -EINTR;
goto out_mm;
}
+ /*
+ * Avoid to modify vma->vm_flags
+ * without locked ops while the
+ * coredump reads the vm_flags.
+ */
+ if (!mmget_still_valid(mm)) {
+ /*
+ * Silently return "count"
+ * like if get_task_mm()
+ * failed. FIXME: should this
+ * function have returned
+ * -ESRCH if get_task_mm()
+ * failed like if
+ * get_proc_task() fails?
+ */
+ up_write(&mm->mmap_sem);
+ goto out_mm;
+ }
for (vma = mm->mmap; vma; vma = vma->vm_next) {
vma->vm_flags &= ~VM_SOFTDIRTY;
vma_set_page_prot(vma);
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 784d667..8bf425a 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -479,6 +479,8 @@ static int userfaultfd_release(struct inode *inode, struct file *file)
* taking the mmap_sem for writing.
*/
down_write(&mm->mmap_sem);
+ if (!mmget_still_valid(mm))
+ goto skip_mm;
prev = NULL;
for (vma = mm->mmap; vma; vma = vma->vm_next) {
cond_resched();
@@ -501,6 +503,7 @@ static int userfaultfd_release(struct inode *inode, struct file *file)
vma->vm_flags = new_flags;
vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
}
+skip_mm:
up_write(&mm->mmap_sem);
mmput(mm);
wakeup:
@@ -802,6 +805,9 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
goto out;
down_write(&mm->mmap_sem);
+ if (!mmget_still_valid(mm))
+ goto out_unlock;
+
vma = find_vma_prev(mm, start, &prev);
if (!vma)
goto out_unlock;
@@ -947,6 +953,9 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
goto out;
down_write(&mm->mmap_sem);
+ if (!mmget_still_valid(mm))
+ goto out_unlock;
+
vma = find_vma_prev(mm, start, &prev);
if (!vma)
goto out_unlock;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e3c8d40..c239984 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1189,6 +1189,27 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long address,
void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
unsigned long start, unsigned long end);
+/*
+ * This has to be called after a get_task_mm()/mmget_not_zero()
+ * followed by taking the mmap_sem for writing before modifying the
+ * vmas or anything the coredump pretends not to change from under it.
+ *
+ * NOTE: find_extend_vma() called from GUP context is the only place
+ * that can modify the "mm" (notably the vm_start/end) under mmap_sem
+ * for reading and outside the context of the process, so it is also
+ * the only case that holds the mmap_sem for reading that must call
+ * this function. Generally if the mmap_sem is hold for reading
+ * there's no need of this check after get_task_mm()/mmget_not_zero().
+ *
+ * This function can be obsoleted and the check can be removed, after
+ * the coredump code will hold the mmap_sem for writing before
+ * invoking the ->core_dump methods.
+ */
+static inline bool mmget_still_valid(struct mm_struct *mm)
+{
+ return likely(!mm->core_state);
+}
+
/**
* mm_walk - callbacks for walk_page_range
* @pmd_entry: if set, called for each non-empty PMD (3rd-level) entry
diff --git a/mm/mmap.c b/mm/mmap.c
index 3f2314a..19368fb 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2448,7 +2448,8 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
vma = find_vma_prev(mm, addr, &prev);
if (vma && (vma->vm_start <= addr))
return vma;
- if (!prev || expand_stack(prev, addr))
+ /* don't alter vm_end if the coredump is running */
+ if (!prev || !mmget_still_valid(mm) || expand_stack(prev, addr))
return NULL;
if (prev->vm_flags & VM_LOCKED)
populate_vma_page_range(prev, addr, prev->vm_end, NULL);
@@ -2474,6 +2475,9 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
return vma;
if (!(vma->vm_flags & VM_GROWSDOWN))
return NULL;
+ /* don't alter vm_start if the coredump is running */
+ if (!mmget_still_valid(mm))
+ return NULL;
start = vma->vm_start;
if (expand_stack(vma, addr))
return NULL;
--
2.7.4
^ permalink raw reply related
* [PATCH v3 2/2][v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
From: Ajay Kaher @ 2019-06-22 13:02 UTC (permalink / raw)
To: aarcange, jannh, oleg, peterx, rppt, jgg, mhocko
Cc: jglisse, akpm, mike.kravetz, viro, riandrews, arve, yishaih,
dledford, sean.hefty, hal.rosenstock, matanb, leonro,
linux-fsdevel, linux-mm, devel, linux-rdma, linux-kernel, stable,
akaher, srivatsab, amakhalov
In-Reply-To: <1561208539-29682-1-git-send-email-akaher@vmware.com>
This patch is the extension of following upstream commit to fix
the race condition between get_task_mm() and core dumping
for IB->mlx4 and IB->mlx5 drivers:
commit 04f5866e41fb ("coredump: fix race condition between
mmget_not_zero()/get_task_mm() and core dumping")'
Thanks to Jason for pointing this.
Signed-off-by: Ajay Kaher <akaher@vmware.com>
---
drivers/infiniband/hw/mlx4/main.c | 4 +++-
drivers/infiniband/hw/mlx5/main.c | 3 +++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 8d59a59..7ccf722 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1172,6 +1172,8 @@ static void mlx4_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
* mlx4_ib_vma_close().
*/
down_write(&owning_mm->mmap_sem);
+ if (!mmget_still_valid(owning_mm))
+ goto skip_mm;
for (i = 0; i < HW_BAR_COUNT; i++) {
vma = context->hw_bar_info[i].vma;
if (!vma)
@@ -1190,7 +1192,7 @@ static void mlx4_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
/* context going to be destroyed, should not access ops any more */
context->hw_bar_info[i].vma->vm_ops = NULL;
}
-
+skip_mm:
up_write(&owning_mm->mmap_sem);
mmput(owning_mm);
put_task_struct(owning_process);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index b1daf5c..f94df0e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -1307,6 +1307,8 @@ static void mlx5_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
* mlx5_ib_vma_close.
*/
down_write(&owning_mm->mmap_sem);
+ if (!mmget_still_valid(owning_mm))
+ goto skip_mm;
list_for_each_entry_safe(vma_private, n, &context->vma_private_list,
list) {
vma = vma_private->vma;
@@ -1321,6 +1323,7 @@ static void mlx5_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
list_del(&vma_private->list);
kfree(vma_private);
}
+skip_mm:
up_write(&owning_mm->mmap_sem);
mmput(owning_mm);
put_task_struct(owning_process);
--
2.7.4
^ permalink raw reply related
* [PATCH v3 0/2] [v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
From: Ajay Kaher @ 2019-06-22 13:02 UTC (permalink / raw)
To: aarcange, jannh, oleg, peterx, rppt, jgg, mhocko
Cc: jglisse, akpm, mike.kravetz, viro, riandrews, arve, yishaih,
dledford, sean.hefty, hal.rosenstock, matanb, leonro,
linux-fsdevel, linux-mm, devel, linux-rdma, linux-kernel, stable,
akaher, srivatsab, amakhalov
In-Reply-To: <1561208539-29682-1-git-send-email-akaher@vmware.com>
coredump: fix race condition between mmget_not_zero()/get_task_mm()
and core dumping
[PATCH v3 1/2]:
Backporting of commit 04f5866e41fb70690e28397487d8bd8eea7d712a upstream.
[PATCH v3 2/2]:
Extension of commit 04f5866e41fb to fix the race condition between
get_task_mm() and core dumping for IB->mlx4 and IB->mlx5 drivers.
[diff from v2]:
- moved mmget_still_valid to mm.h in [PATCH v3 1/2]
- added binder.c changes in [PATCH v3 1/2]
- added [PATCH v3 2/2]
^ permalink raw reply
* [PATCH iproute2-next v3 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: Denis Kirjanov @ 2019-06-22 18:00 UTC (permalink / raw)
To: stephen, dsahern; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
Current code assumes that we print ethernet mac and
that doesn't work in the IPoIB case with SRIOV-enabled hardware
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: updated kernel headers to uapi commit
v2->v3: fixed alignment
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
ip/ipaddress.c | 44 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b504200b..52078675 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -26,6 +26,7 @@
#include <linux/netdevice.h>
#include <linux/if_arp.h>
+#include <linux/if_infiniband.h>
#include <linux/sockios.h>
#include <linux/net_namespace.h>
@@ -349,9 +350,10 @@ static void print_af_spec(FILE *fp, struct rtattr *af_spec_attr)
static void print_vf_stats64(FILE *fp, struct rtattr *vfstats);
-static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
+static void print_vfinfo(struct ifinfomsg *ifi, FILE *fp, struct rtattr *vfinfo)
{
struct ifla_vf_mac *vf_mac;
+ struct ifla_vf_broadcast *vf_broadcast;
struct ifla_vf_tx_rate *vf_tx_rate;
struct rtattr *vf[IFLA_VF_MAX + 1] = {};
@@ -365,13 +367,45 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
+ vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
print_string(PRINT_FP, NULL, "%s ", _SL_);
print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
- print_string(PRINT_ANY, "mac", "MAC %s",
- ll_addr_n2a((unsigned char *) &vf_mac->mac,
- ETH_ALEN, 0, b1, sizeof(b1)));
+
+ print_string(PRINT_ANY,
+ "link_type",
+ " link/%s ",
+ ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
+
+ print_color_string(PRINT_ANY,
+ COLOR_MAC,
+ "address",
+ "%s",
+ ll_addr_n2a((unsigned char *) &vf_mac->mac,
+ ifi->ifi_type == ARPHRD_ETHER ?
+ ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+
+ if (vf[IFLA_VF_BROADCAST]) {
+ if (ifi->ifi_flags&IFF_POINTOPOINT) {
+ print_string(PRINT_FP, NULL, " peer ", NULL);
+ print_bool(PRINT_JSON,
+ "link_pointtopoint", NULL, true);
+ } else
+ print_string(PRINT_FP, NULL, " brd ", NULL);
+
+ print_color_string(PRINT_ANY,
+ COLOR_MAC,
+ "broadcast",
+ "%s",
+ ll_addr_n2a((unsigned char *) &vf_broadcast->broadcast,
+ ifi->ifi_type == ARPHRD_ETHER ?
+ ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+ }
if (vf[IFLA_VF_VLAN_LIST]) {
struct rtattr *i, *vfvlanlist = vf[IFLA_VF_VLAN_LIST];
@@ -1102,7 +1136,7 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
open_json_array(PRINT_JSON, "vfinfo_list");
for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
open_json_object(NULL);
- print_vfinfo(fp, i);
+ print_vfinfo(ifi, fp, i);
close_json_object();
}
close_json_array(PRINT_JSON, NULL);
--
2.12.3
^ permalink raw reply related
* [PATCH iproute2-next v3 2/2] uapi: update if_link.h
From: Denis Kirjanov @ 2019-06-22 18:00 UTC (permalink / raw)
To: stephen, dsahern; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
In-Reply-To: <20190622180035.40245-1-dkirjanov@suse.com>
update if_link.h to commit 75345f888f700c4ab2448287e35d48c760b202e6
("ipoib: show VF broadcast address")
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bfe7f9e6..5f271d84 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -692,6 +692,7 @@ enum {
IFLA_VF_IB_NODE_GUID, /* VF Infiniband node GUID */
IFLA_VF_IB_PORT_GUID, /* VF Infiniband port GUID */
IFLA_VF_VLAN_LIST, /* nested list of vlans, option for QinQ */
+ IFLA_VF_BROADCAST, /* VF broadcast */
__IFLA_VF_MAX,
};
@@ -702,6 +703,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
--
2.12.3
^ permalink raw reply related
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-24 7:27 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-kernel, linux-block, linux-nvme, linux-pci, linux-rdma,
Jens Axboe, Christoph Hellwig, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <20190620161240.22738-1-logang@deltatee.com>
This is not going to fly.
For one passing a dma_addr_t through the block layer is a layering
violation, and one that I think will also bite us in practice.
The host physical to PCIe bus address mapping can have offsets, and
those offsets absolutely can be different for differnet root ports.
So with your caller generated dma_addr_t everything works fine with
a switched setup as the one you are probably testing on, but on a
sufficiently complicated setup with multiple root ports it can break.
Also duplicating the whole block I/O stack, including hooks all over
the fast path is pretty much a no-go.
I've been pondering for a while if we wouldn't be better off just
passing a phys_addr_t + len instead of the page, offset, len tuple
in the bio_vec, though. If you look at the normal I/O path here
is what we normally do:
- we get a page as input, either because we have it at hand (e.g.
from the page cache) or from get_user_pages (which actually caculates
it from a pfn in the page tables)
- once in the bio all the merging decisions are based on the physical
address, so we have to convert it to the physical address there,
potentially multiple times
- then dma mapping all works off the physical address, which it gets
from the page at the start
- then only the dma address is used for the I/O
- on I/O completion we often but not always need the page again. In
the direct I/O case for reference counting and dirty status, in the
file system also for things like marking the page uptodate
So if we move to a phys_addr_t we'd need to go back to the page at least
once. But because of how the merging works we really only need to do
it once per segment, as we can just do pointer arithmerics do get the
following pages. As we generally go at least once from a physical
address to a page in the merging code even a relatively expensive vmem_map
looks shouldn't be too bad. Even more so given that the super hot path
(small blkdev direct I/O) can actually trivially cache the affected pages
as well.
Linus kinda hates the pfn approach, but part of that was really that
it was proposed for file system data, which we all found out really
can't work as-is without pages the hard way. Another part probably
was potential performance issue, but between the few page lookups, and
the fact that using a single phys_addr_t instead of pfn/page + offset
should avoid quite a few calculations performance should not actually
be affected, although we'll have to be careful to actually verify that.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-24 7:31 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Dan Williams, Logan Gunthorpe, Linux Kernel Mailing List,
linux-block, linux-nvme, linux-pci, linux-rdma, Jens Axboe,
Christoph Hellwig, Bjorn Helgaas, Sagi Grimberg, Keith Busch,
Stephen Bates
In-Reply-To: <20190620193353.GF19891@ziepe.ca>
On Thu, Jun 20, 2019 at 04:33:53PM -0300, Jason Gunthorpe wrote:
> > My primary concern with this is that ascribes a level of generality
> > that just isn't there for peer-to-peer dma operations. "Peer"
> > addresses are not "DMA" addresses, and the rules about what can and
> > can't do peer-DMA are not generically known to the block layer.
>
> ?? The P2P infrastructure produces a DMA bus address for the
> initiating device that is is absolutely a DMA address. There is some
> intermediate CPU centric representation, but after mapping it is the
> same as any other DMA bus address.
>
> The map function can tell if the device pair combination can do p2p or
> not.
At the PCIe level there is no such thing as a DMA address, it all
is bus address with MMIO and DMA in the same address space (without
that P2P would have not chance of actually working obviously). But
that bus address space is different per "bus" (which would be an
root port in PCIe), and we need to be careful about that.
^ permalink raw reply
* [PATCH v4 rdma-next 0/3] RDMA/qedr: Use the doorbell overflow recovery mechanism for RDMA
From: Michal Kalderon @ 2019-06-24 10:28 UTC (permalink / raw)
To: michal.kalderon, ariel.elior, jgg, dledford; +Cc: linux-rdma, davem, netdev
This patch series used the doorbell overflow recovery mechanism
introduced in
commit 36907cd5cd72 ("qed: Add doorbell overflow recovery mechanism")
for rdma ( RoCE and iWARP )
rdma-core pull request #493
Changes from V3:
- Remove casts from void to u8. Pointer arithmetic can be done on void
- rebase to tip of rdma-next
Changes from V2:
- Don't use long-lived kmap. Instead use user-trigger mmap for the
doorbell recovery entries.
- Modify dpi_addr to be denoted with __iomem and avoid redundant
casts
Changes from V1:
- call kmap to map virtual address into kernel space
- modify db_rec_delete to be void
- remove some cpu_to_le16 that were added to previous patch which are
correct but not related to the overflow recovery mechanism. Will be
submitted as part of a different patch
Michal Kalderon (3):
qed*: Change dpi_addr to be denoted with __iomem
RDMA/qedr: Add doorbell overflow recovery support
RDMA/qedr: Add iWARP doorbell recovery support
drivers/infiniband/hw/qedr/main.c | 2 +-
drivers/infiniband/hw/qedr/qedr.h | 27 +-
drivers/infiniband/hw/qedr/verbs.c | 386 ++++++++++++++++++++++++-----
drivers/net/ethernet/qlogic/qed/qed_rdma.c | 5 +-
include/linux/qed/qed_rdma_if.h | 2 +-
include/uapi/rdma/qedr-abi.h | 25 ++
6 files changed, 376 insertions(+), 71 deletions(-)
--
2.14.5
^ permalink raw reply
* [PATCH v4 rdma-next 1/3] qed*: Change dpi_addr to be denoted with __iomem
From: Michal Kalderon @ 2019-06-24 10:28 UTC (permalink / raw)
To: michal.kalderon, ariel.elior, jgg, dledford; +Cc: linux-rdma, davem, netdev
In-Reply-To: <20190624102809.8793-1-michal.kalderon@marvell.com>
Several casts were required around dpi_addr parameter in qed_rdma_if.h
This is an address on the doorbell bar and should therefore be marked
with __iomem.
Reported-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
drivers/infiniband/hw/qedr/main.c | 2 +-
drivers/infiniband/hw/qedr/qedr.h | 2 +-
drivers/net/ethernet/qlogic/qed/qed_rdma.c | 5 ++---
include/linux/qed/qed_rdma_if.h | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index a0a7ba0a5af4..3db4b6ba5ad6 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ b/drivers/infiniband/hw/qedr/main.c
@@ -815,7 +815,7 @@ static int qedr_init_hw(struct qedr_dev *dev)
if (rc)
goto out;
- dev->db_addr = (void __iomem *)(uintptr_t)out_params.dpi_addr;
+ dev->db_addr = out_params.dpi_addr;
dev->db_phys_addr = out_params.dpi_phys_addr;
dev->db_size = out_params.dpi_size;
dev->dpi = out_params.dpi;
diff --git a/drivers/infiniband/hw/qedr/qedr.h b/drivers/infiniband/hw/qedr/qedr.h
index 6175d1e98717..8df56aba9d2c 100644
--- a/drivers/infiniband/hw/qedr/qedr.h
+++ b/drivers/infiniband/hw/qedr/qedr.h
@@ -227,7 +227,7 @@ struct qedr_ucontext {
struct ib_ucontext ibucontext;
struct qedr_dev *dev;
struct qedr_pd *pd;
- u64 dpi_addr;
+ void __iomem *dpi_addr;
u64 dpi_phys_addr;
u32 dpi_size;
u16 dpi;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index 7873d6dfd91f..fb3fe60a1a68 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
@@ -799,9 +799,8 @@ static int qed_rdma_add_user(void *rdma_cxt,
/* Calculate the corresponding DPI address */
dpi_start_offset = p_hwfn->dpi_start_offset;
- out_params->dpi_addr = (u64)((u8 __iomem *)p_hwfn->doorbells +
- dpi_start_offset +
- ((out_params->dpi) * p_hwfn->dpi_size));
+ out_params->dpi_addr = p_hwfn->doorbells + dpi_start_offset +
+ out_params->dpi * p_hwfn->dpi_size;
out_params->dpi_phys_addr = p_hwfn->cdev->db_phys_addr +
dpi_start_offset +
diff --git a/include/linux/qed/qed_rdma_if.h b/include/linux/qed/qed_rdma_if.h
index d15f8e4815e3..834166809a6c 100644
--- a/include/linux/qed/qed_rdma_if.h
+++ b/include/linux/qed/qed_rdma_if.h
@@ -225,7 +225,7 @@ struct qed_rdma_start_in_params {
struct qed_rdma_add_user_out_params {
u16 dpi;
- u64 dpi_addr;
+ void __iomem *dpi_addr;
u64 dpi_phys_addr;
u32 dpi_size;
u16 wid_count;
--
2.14.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox