| Message ID | 1526392437-20924-1-git-send-email-awallis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 15, 2018 at 09:53:57AM -0400, Adam Wallis wrote: > The xhci driver forces DMA memory to be node aware, however, there are > several ring-related memory allocations that are not memory node aware. > This patch resolves those *alloc functions to be allocated on the proper > memory node. Does this really do anything? Given the speed of USB3 at the moment, does fixing the memory to the node the PCI device is on show any measurable speedups? Last I remember about NUMA systems, it wasn't always a win depending on where the irq came in from, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/15/2018 11:07 AM, Greg Kroah-Hartman wrote: > On Tue, May 15, 2018 at 09:53:57AM -0400, Adam Wallis wrote: > Does this really do anything? Given the speed of USB3 at the moment, > does fixing the memory to the node the PCI device is on show any > measurable speedups? Last I remember about NUMA systems, it wasn't > always a win depending on where the irq came in from, right? > > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > I was getting really inconsistent throughput speeds on a system I was testing with NUMA nodes. Using an SMMU in identity mode, I was able to track down where the performance deltas were coming from...Some of the rings were going to the "wrong" node. Yes, it's possible to handle your IRQs with CPUs on the wrong NUMA node...but I would argue that it's always best to have the rings for USB controller X as close to controller X if possible. Users can then properly constrain IRQs, and even kernel threads to the right Domain if they so desire. After setting the IRQ affinity to the right node AND applying this patch, I started getting much more reliable (and faster) results.
On Tue, May 15, 2018 at 04:51:53PM -0400, Adam Wallis wrote: > On 5/15/2018 11:07 AM, Greg Kroah-Hartman wrote: > > On Tue, May 15, 2018 at 09:53:57AM -0400, Adam Wallis wrote: > > Does this really do anything? Given the speed of USB3 at the moment, > > does fixing the memory to the node the PCI device is on show any > > measurable speedups? Last I remember about NUMA systems, it wasn't > > always a win depending on where the irq came in from, right? > > > > thanks, > > > > greg k-h > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > I was getting really inconsistent throughput speeds on a system I was testing > with NUMA nodes. Using an SMMU in identity mode, I was able to track down where > the performance deltas were coming from...Some of the rings were going to the > "wrong" node. > > Yes, it's possible to handle your IRQs with CPUs on the wrong NUMA node...but I > would argue that it's always best to have the rings for USB controller X as > close to controller X if possible. Users can then properly constrain IRQs, and > even kernel threads to the right Domain if they so desire. > > After setting the IRQ affinity to the right node AND applying this patch, I > started getting much more reliable (and faster) results. Ok, fair enough, I was hoping that "modern" systems would have better NUMA memory interconnects. I guess that isn't the case still :( thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/16/2018 2:02 AM, Greg Kroah-Hartman wrote: > On Tue, May 15, 2018 at 04:51:53PM -0400, Adam Wallis wrote: > > Ok, fair enough, I was hoping that "modern" systems would have better > NUMA memory interconnects. I guess that isn't the case still :( Things will keep improving, I'm sure. Mathias, did you have anything you need reworked with this patch or any specific comments on the overall patch? Thanks!
On 21.05.2018 15:56, Adam Wallis wrote: > On 5/16/2018 2:02 AM, Greg Kroah-Hartman wrote: >> On Tue, May 15, 2018 at 04:51:53PM -0400, Adam Wallis wrote: >> >> Ok, fair enough, I was hoping that "modern" systems would have better >> NUMA memory interconnects. I guess that isn't the case still :( > > Things will keep improving, I'm sure. > > Mathias, did you have anything you need reworked with this patch or any specific > comments on the overall patch? > If there's a performance benefit in adding these then I'm all for it. for ring related allocations it makes sense. Not sure if if there's any benefit in allocating the scratchpad structures from a closer node, or any harm? xhci driver doesn't really access scratchpad that frequently. The port related structures are changing completely, so xhci->usb2_ports xhci->usb3_ports and other related are going away. Just sent a series for that. So some rebasing is needed. -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/21/2018 9:53 AM, Mathias Nyman wrote: > On 21.05.2018 15:56, Adam Wallis wrote: > Not sure if if there's any benefit in allocating the scratchpad structures from > a closer node, or any harm? xhci driver doesn't really access scratchpad that > frequently. I don't see how it would hurt and I think it's easier to keep all of the allocations consistent...however, let me know if you want those backed out. > The port related structures are changing completely, so xhci->usb2_ports > xhci->usb3_ports > and other related are going away. Just sent a series for that. So some rebasing > is needed. Would it be easier to just rebase on top of your for-usb-next branch in that case? I had been only testing on top of 4.17rc5. Let me know what you think is best/easiest for you.
On 21.05.2018 19:42, Adam Wallis wrote: > On 5/21/2018 9:53 AM, Mathias Nyman wrote: >> On 21.05.2018 15:56, Adam Wallis wrote: >> Not sure if if there's any benefit in allocating the scratchpad structures from >> a closer node, or any harm? xhci driver doesn't really access scratchpad that >> frequently. > > I don't see how it would hurt and I think it's easier to keep all of the > allocations consistent...however, let me know if you want those backed out. I guess they don't do any harm > >> The port related structures are changing completely, so xhci->usb2_ports >> xhci->usb3_ports >> and other related are going away. Just sent a series for that. So some rebasing >> is needed. > > Would it be easier to just rebase on top of your for-usb-next branch in that > case? I had been only testing on top of 4.17rc5. > > Let me know what you think is best/easiest for you. > Rebasing on top of my for-usb-next would be easiest for me Thanks Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 332420d..4a229f5 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -33,8 +33,9 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci, struct xhci_segment *seg; dma_addr_t dma; int i; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; - seg = kzalloc(sizeof *seg, flags); + seg = kzalloc_node(sizeof(*seg), flags, dev_to_node(dev)); if (!seg) return NULL; @@ -45,7 +46,8 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci, } if (max_packet) { - seg->bounce_buf = kzalloc(max_packet, flags); + seg->bounce_buf = kzalloc_node(max_packet, flags, + dev_to_node(dev)); if (!seg->bounce_buf) { dma_pool_free(xhci->segment_pool, seg->trbs, dma); kfree(seg); @@ -363,8 +365,9 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, { struct xhci_ring *ring; int ret; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; - ring = kzalloc(sizeof *(ring), flags); + ring = kzalloc_node(sizeof(*ring), flags, dev_to_node(dev)); if (!ring) return NULL; @@ -458,11 +461,12 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci, int type, gfp_t flags) { struct xhci_container_ctx *ctx; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; if ((type != XHCI_CTX_TYPE_DEVICE) && (type != XHCI_CTX_TYPE_INPUT)) return NULL; - ctx = kzalloc(sizeof(*ctx), flags); + ctx = kzalloc_node(sizeof(*ctx), flags, dev_to_node(dev)); if (!ctx) return NULL; @@ -615,6 +619,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci, struct xhci_ring *cur_ring; u64 addr; int ret; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; xhci_dbg(xhci, "Allocating %u streams and %u " "stream context array entries.\n", @@ -625,7 +630,8 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci, } xhci->cmd_ring_reserved_trbs++; - stream_info = kzalloc(sizeof(struct xhci_stream_info), mem_flags); + stream_info = kzalloc_node(sizeof(*stream_info), mem_flags, + dev_to_node(dev)); if (!stream_info) goto cleanup_trbs; @@ -633,9 +639,11 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci, stream_info->num_stream_ctxs = num_stream_ctxs; /* Initialize the array of virtual pointers to stream rings. */ - stream_info->stream_rings = kzalloc( - sizeof(struct xhci_ring *)*num_streams, - mem_flags); + stream_info->stream_rings = kcalloc_node( + num_streams, + sizeof(struct xhci_ring *), + mem_flags, + dev_to_node(dev)); if (!stream_info->stream_rings) goto cleanup_info; @@ -831,6 +839,7 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci, struct xhci_tt_bw_info *tt_info; unsigned int num_ports; int i, j; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; if (!tt->multi) num_ports = 1; @@ -840,7 +849,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci, for (i = 0; i < num_ports; i++, tt_info++) { struct xhci_interval_bw_table *bw_table; - tt_info = kzalloc(sizeof(*tt_info), mem_flags); + tt_info = kzalloc_node(sizeof(*tt_info), mem_flags, + dev_to_node(dev)); if (!tt_info) goto free_tts; INIT_LIST_HEAD(&tt_info->tt_list); @@ -1640,7 +1650,8 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags) if (!num_sp) return 0; - xhci->scratchpad = kzalloc(sizeof(*xhci->scratchpad), flags); + xhci->scratchpad = kzalloc_node(sizeof(*xhci->scratchpad), flags, + dev_to_node(dev)); if (!xhci->scratchpad) goto fail_sp; @@ -1650,7 +1661,8 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags) if (!xhci->scratchpad->sp_array) goto fail_sp2; - xhci->scratchpad->sp_buffers = kzalloc(sizeof(void *) * num_sp, flags); + xhci->scratchpad->sp_buffers = kcalloc_node(num_sp, sizeof(void *), + flags, dev_to_node(dev)); if (!xhci->scratchpad->sp_buffers) goto fail_sp3; @@ -1718,14 +1730,16 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci, bool allocate_completion, gfp_t mem_flags) { struct xhci_command *command; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; - command = kzalloc(sizeof(*command), mem_flags); + command = kzalloc_node(sizeof(*command), mem_flags, dev_to_node(dev)); if (!command) return NULL; if (allocate_completion) { command->completion = - kzalloc(sizeof(struct completion), mem_flags); + kzalloc_node(sizeof(struct completion), mem_flags, + dev_to_node(dev)); if (!command->completion) { kfree(command); return NULL; @@ -2098,6 +2112,7 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports, int i; u8 major_revision, minor_revision; struct xhci_hub *rhub; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; temp = readl(addr); major_revision = XHCI_EXT_PORT_MAJOR(temp); @@ -2134,8 +2149,8 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports, rhub->psi_count = XHCI_EXT_PORT_PSIC(temp); if (rhub->psi_count) { - rhub->psi = kcalloc(rhub->psi_count, sizeof(*rhub->psi), - GFP_KERNEL); + rhub->psi = kcalloc_node(rhub->psi_count, sizeof(*rhub->psi), + GFP_KERNEL, dev_to_node(dev)); if (!rhub->psi) rhub->psi_count = 0; @@ -2229,13 +2244,16 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) int i, j, port_index; int cap_count = 0; u32 cap_start; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; num_ports = HCS_MAX_PORTS(xhci->hcs_params1); - xhci->port_array = kzalloc(sizeof(*xhci->port_array)*num_ports, flags); + xhci->port_array = kcalloc_node(num_ports, sizeof(*xhci->port_array), + flags, dev_to_node(dev)); if (!xhci->port_array) return -ENOMEM; - xhci->rh_bw = kzalloc(sizeof(*xhci->rh_bw)*num_ports, flags); + xhci->rh_bw = kcalloc_node(num_ports, sizeof(*xhci->rh_bw), flags, + dev_to_node(dev)); if (!xhci->rh_bw) return -ENOMEM; for (i = 0; i < num_ports; i++) { @@ -2262,7 +2280,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) XHCI_EXT_CAPS_PROTOCOL); } - xhci->ext_caps = kzalloc(sizeof(*xhci->ext_caps) * cap_count, flags); + xhci->ext_caps = kcalloc_node(cap_count, sizeof(*xhci->ext_caps), + flags, dev_to_node(dev)); if (!xhci->ext_caps) return -ENOMEM; @@ -2305,8 +2324,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) * Not sure how the USB core will handle a hub with no ports... */ if (xhci->num_usb2_ports) { - xhci->usb2_ports = kmalloc(sizeof(*xhci->usb2_ports)* - xhci->num_usb2_ports, flags); + xhci->usb2_ports = kcalloc_node(xhci->num_usb2_ports, + sizeof(*xhci->usb2_ports), flags, dev_to_node(dev)); if (!xhci->usb2_ports) return -ENOMEM; @@ -2330,8 +2349,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) } } if (xhci->num_usb3_ports) { - xhci->usb3_ports = kmalloc(sizeof(*xhci->usb3_ports)* - xhci->num_usb3_ports, flags); + xhci->usb3_ports = kcalloc_node(xhci->num_usb3_ports, + sizeof(*xhci->usb3_ports), flags, dev_to_node(dev)); if (!xhci->usb3_ports) return -ENOMEM;
The xhci driver forces DMA memory to be node aware, however, there are several ring-related memory allocations that are not memory node aware. This patch resolves those *alloc functions to be allocated on the proper memory node. Signed-off-by: Adam Wallis <awallis@codeaurora.org> --- drivers/usb/host/xhci-mem.c | 63 +++++++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 22 deletions(-)