sfc: NUMA support
Subsystems (3)
cover sfc: NUMA support
This patch series adds support for
- allocating rx pages local to the interrupt
- setting affinity hint to influence IRQs to be allocated on the same NUMA node as the one where the card resides.
Alexandra Kossovsky (1):
sfc: use __GFP_NOWARN when allocating RX pages from atomic context.
Bert Kenward (2):
sfc: Use cpu_to_mem() to support memoryless nodes
sfc: set and clear interrupt affinity hints
Daniel Pieczko (1):
sfc: allocate rx pages on the same node as the interrupt
drivers/net/ethernet/sfc/efx.c | 36 +++++++++++++++++++++++++++++++++++ drivers/net/ethernet/sfc/net_driver.h | 3 +++ drivers/net/ethernet/sfc/rx.c | 18 +++++++++++++++--- 3 files changed, 54 insertions(+), 3 deletions(-)
PATCH 2/4 sfc: allocate rx pages on the same node as the interrupt
From: Daniel Pieczko <[email protected]>
When the interrupt servicing a channel is on a NUMA node that is not local to the device, performance is improved by allocating rx pages on the node local to the interrupt (remote to the device)
The performance-optimal case, where interrupts and applications are pinned to CPUs on the same node as the device, is not altered by this change.
This change gave a 1% improvement in transaction rate using Nginx with all interrupts and Nginx threads on the node remote to the device. It also gave a small reduction in round-trip latency, again with the interrupt and application on a different node to the device.
Allocating rx pages based on the channel->irq_node value is only valid for the initial driver-load interrupt affinities; if an interrupt is moved later, the wrong node may be used for the allocation.
--- drivers/net/ethernet/sfc/efx.c | 1 + drivers/net/ethernet/sfc/net_driver.h | 3 +++ drivers/net/ethernet/sfc/rx.c | 14 +++++++++----- 3 files changed, 13 insertions(+), 5 deletions(-)
drivers/net/ethernet/sfc/efx.c +1 -0
@@ -445,6 +445,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel) channel->efx = efx; channel->channel = i; channel->type = &efx_default_channel_type; + channel->irq_node =NUMA_NO_NODE ;for (j =0 ; j <EFX_TXQ_TYPES ; j++) { tx_queue = &channel->tx_queue[j];
drivers/net/ethernet/sfc/net_driver.h +3 -0
@@ -419,6 +419,7 @@ enum efx_sync_events_state {* @sync_events_state: Current state of sync events on this channel * @sync_timestamp_major: Major part of the last ptp sync event * @sync_timestamp_minor: Minor part of the last ptp sync event +* @irq_node: NUMA node of interrupt */ struct efx_channel {struct efx_nic *efx;
@@ -477,6 +478,8 @@ struct efx_channel {enum efx_sync_events_state sync_events_state;u32 sync_timestamp_major;u32 sync_timestamp_minor; + +int irq_node; };#ifdef CONFIG_NET_RX_BUSY_POLL
drivers/net/ethernet/sfc/rx.c +9 -5
@@ -168,11 +168,15 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)* context in such a case. So, use __GFP_NO_WARN * in case of atomic. */ - page =alloc_pages (__GFP_COLD | __GFP_COMP | - (atomic ? - (GFP_ATOMIC | __GFP_NOWARN) - :GFP_KERNEL ), - efx->rx_buffer_order); +struct efx_channel *channel; + + channel =efx_rx_queue_channel (rx_queue); + page =alloc_pages_node (channel->irq_node, __GFP_COMP | + (atomic ? + (GFP_ATOMIC | __GFP_NOWARN) + :GFP_KERNEL ), + efx->rx_buffer_order); +if (unlikely (page ==NULL ))return -ENOMEM ; dma_addr =
PATCH 3/4 sfc: Use cpu_to_mem() to support memoryless nodes
In reply to Shradha ShahFrom: Bert Kenward <[email protected]>
With CONFIG_HAVE_MEMORYLESS_NODES cpu_to_node() may return nodes without memory, which is not a good choice when later using that to allocate memory. cpu_to_mem() instead provides the most appropriate NUMA node to allocate from.
--- drivers/net/ethernet/sfc/efx.c | 2 +- drivers/net/ethernet/sfc/net_driver.h | 4 ++-- drivers/net/ethernet/sfc/rx.c | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-)
drivers/net/ethernet/sfc/efx.c +1 -1
@@ -445,7 +445,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel) channel->efx = efx; channel->channel = i; channel->type = &efx_default_channel_type; - channel->irq_node =NUMA_NO_NODE ; + channel->irq_mem_node =NUMA_NO_NODE ;for (j =0 ; j <EFX_TXQ_TYPES ; j++) { tx_queue = &channel->tx_queue[j];
drivers/net/ethernet/sfc/net_driver.h +2 -2
@@ -419,7 +419,7 @@ enum efx_sync_events_state {* @sync_events_state: Current state of sync events on this channel * @sync_timestamp_major: Major part of the last ptp sync event * @sync_timestamp_minor: Minor part of the last ptp sync event -* @irq_node: NUMA node of interrupt +* @irq_mem_node: Memory NUMA node of interrupt */ struct efx_channel {struct efx_nic *efx;
@@ -479,7 +479,7 @@ struct efx_channel {u32 sync_timestamp_major;u32 sync_timestamp_minor; -int irq_node; +int irq_mem_node; };#ifdef CONFIG_NET_RX_BUSY_POLL
drivers/net/ethernet/sfc/rx.c +2 -1
@@ -171,7 +171,8 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)struct efx_channel *channel; channel =efx_rx_queue_channel (rx_queue); - page = alloc_pages_node(channel->irq_node, __GFP_COMP | + page = alloc_pages_node(channel->irq_mem_node, + __GFP_COMP | (atomic ? (GFP_ATOMIC | __GFP_NOWARN) : GFP_KERNEL),
PATCH 1/4 sfc: use __GFP_NOWARN when allocating RX pages from atomic context.
In reply to Shradha ShahFrom: Alexandra Kossovsky <[email protected]>
If we fail to allocate a page when in atomic context this is handled by scheduling a fill in non-atomic context. As such, a warning is not needed.
--- drivers/net/ethernet/sfc/rx.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/net/ethernet/sfc/rx.c +8 -1
@@ -163,8 +163,15 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic) do { page = efx_reuse_page(rx_queue); if (page == NULL) { + /* GFP_ATOMIC may fail because of various reasons, + * and we re-schedule rx_fill from non-atomic + * context in such a case. So, use __GFP_NO_WARN + * in case of atomic. + */ page = alloc_pages(__GFP_COLD | __GFP_COMP | - (atomic ? GFP_ATOMIC : GFP_KERNEL), + (atomic ? + (GFP_ATOMIC | __GFP_NOWARN) + : GFP_KERNEL), efx->rx_buffer_order); if (unlikely(page == NULL)) return -ENOMEM;
PATCH 4/4 sfc: set and clear interrupt affinity hints
In reply to Shradha ShahFrom: Bert Kenward <[email protected]>
Use cpumask_local_spread to provide interrupt affinity hints for each queue. This will spread interrupts across NUMA local CPUs first, extending to remote nodes if needed.
--- drivers/net/ethernet/sfc/efx.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
drivers/net/ethernet/sfc/efx.c +35 -0
@@ -1489,6 +1489,30 @@ static int efx_probe_interrupts(struct efx_nic *efx)return 0 ; } +#if defined(CONFIG_SMP ) +static void efx_set_interrupt_affinity (struct efx_nic *efx) +{ +struct efx_channel *channel; +unsigned int cpu; + +efx_for_each_channel (channel, efx) { + cpu =cpumask_local_spread (channel->channel, +pcibus_to_node (efx->pci_dev->bus)); + +irq_set_affinity_hint (channel->irq,cpumask_of (cpu)); + channel->irq_mem_node =cpu_to_mem (cpu); + } +} + +static void efx_clear_interrupt_affinity (struct efx_nic *efx) +{ +struct efx_channel *channel; + +efx_for_each_channel (channel, efx) +irq_set_affinity_hint (channel->irq,NULL ); +} +#endif /* CONFIG_SMP */ +static int efx_soft_enable_interrupts (struct efx_nic *efx) {struct efx_channel *channel, *end_channel;
@@ -2932,6 +2956,9 @@ static void efx_pci_remove_main(struct efx_nic *efx)cancel_work_sync (&efx->reset_work);efx_disable_interrupts (efx); +#if defined(CONFIG_SMP ) +efx_clear_interrupt_affinity (efx); +#endif efx_nic_fini_interrupt (efx);efx_fini_port (efx); efx->type->fini (efx);
@@ -3081,6 +3108,11 @@ static int efx_pci_probe_main(struct efx_nic *efx) rc =efx_nic_init_interrupt (efx);if (rc) goto fail5; + +#if defined(CONFIG_SMP ) +efx_set_interrupt_affinity (efx); +#endif + rc =efx_enable_interrupts (efx);if (rc) goto fail6;
@@ -3088,6 +3120,9 @@ static int efx_pci_probe_main(struct efx_nic *efx)return 0 ; fail6: +#if defined(CONFIG_SMP ) +efx_clear_interrupt_affinity (efx); +#endif efx_nic_fini_interrupt (efx); fail5:efx_fini_port (efx);
Re: [PATCH net-next 2/4] sfc: allocate rx pages on the same node as the interrupt
In reply to Shradha ShahQuoted 77 lines · Shradha Shah
From: Daniel Pieczko <[email protected]>
When the interrupt servicing a channel is on a NUMA node that is not local to the device, performance is improved by allocating rx pages on the node local to the interrupt (remote to the device)
The performance-optimal case, where interrupts and applications are pinned to CPUs on the same node as the device, is not altered by this change.
This change gave a 1% improvement in transaction rate using Nginx with all interrupts and Nginx threads on the node remote to the device. It also gave a small reduction in round-trip latency, again with the interrupt and application on a different node to the device.
Allocating rx pages based on the
channel->irq_nodevalue is only valid for the initial driver-load interrupt affinities; if an interrupt is moved later, the wrong node may be used for the allocation.Signed-off-by Shradha Shah <[email protected]>--- drivers/net/ethernet/sfc/efx.c | 1 + drivers/net/ethernet/sfc/net_driver.h | 3 +++ drivers/net/ethernet/sfc/rx.c | 14 +++++++++----- 3 files changed, 13 insertions(+), 5 deletions(-)drivers/net/ethernet/sfc/efx.c +1
-0@@ -445,6 +445,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel) channel->efx = efx; channel->channel = i; channel->type = &efx_default_channel_type; + channel->irq_node =NUMA_NO_NODE ;for (j =0 ; j <EFX_TXQ_TYPES ; j++) { tx_queue = &channel->tx_queue[j];drivers/net/ethernet/sfc/net_driver.h +3
-0@@ -419,6 +419,7 @@ enum efx_sync_events_state {* @sync_events_state: Current state of sync events on this channel * @sync_timestamp_major: Major part of the last ptp sync event * @sync_timestamp_minor: Minor part of the last ptp sync event +* @irq_node: NUMA node of interrupt */ struct efx_channel {struct efx_nic *efx;@@ -477,6 +478,8 @@ struct efx_channel {enum efx_sync_events_state sync_events_state;u32 sync_timestamp_major;u32 sync_timestamp_minor; + +int irq_node; };#ifdef CONFIG_NET_RX_BUSY_POLL drivers/net/ethernet/sfc/rx.c +2
-5@@ -168,11 +168,15 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)* context in such a case. So, use __GFP_NO_WARN * in case of atomic. */ - page =alloc_pages (__GFP_COLD | __GFP_COMP | - (atomic ? - (GFP_ATOMIC | __GFP_NOWARN) - :GFP_KERNEL ), - efx->rx_buffer_order); +struct efx_channel *channel; +
drivers/net/ethernet/sfc/rx.c static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic) · line 182
Quoted 10 lines
drivers/net/ethernet/sfc/rx.c +7
-0+ channel =efx_rx_queue_channel (rx_queue); + page =alloc_pages_node (channel->irq_node, __GFP_COMP | + (atomic ? + (GFP_ATOMIC | __GFP_NOWARN) + :GFP_KERNEL ), + efx->rx_buffer_order); +if (unlikely (page ==NULL ))return -ENOMEM ; dma_addr =
Sorry, I do not understand this patch, and why the following one is not squashed on this one.
irq_node is always NUMA_NO_NODE (in this patch)
So you claim a 1% improvement, switching from alloc_pages(...) to alloc_pages_node(NUMA_NO_NODE, ...) ???
Re: [PATCH net-next 4/4] sfc: set and clear interrupt affinity hints
In reply to Shradha ShahHello.
Quoted 47 lines · Shradha Shah
From: Bert Kenward <[email protected]>
Use
cpumask_local_spreadto provide interrupt affinity hints for each queue. This will spread interrupts across NUMA local CPUs first, extending to remote nodes if needed.Signed-off-by Shradha Shah <[email protected]>--- drivers/net/ethernet/sfc/efx.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/net/ethernet/sfc/efx.c +24
-0@@ -1489,6 +1489,30 @@ static int efx_probe_interrupts(struct efx_nic *efx)return 0 ; } +#if defined(CONFIG_SMP ) +static void efx_set_interrupt_affinity (struct efx_nic *efx) +{ +struct efx_channel *channel; +unsigned int cpu; + +efx_for_each_channel (channel, efx) { + cpu =cpumask_local_spread (channel->channel, +pcibus_to_node (efx->pci_dev->bus)); + +irq_set_affinity_hint (channel->irq,cpumask_of (cpu)); + channel->irq_mem_node =cpu_to_mem (cpu); + } +} + +static void efx_clear_interrupt_affinity (struct efx_nic *efx) +{ +struct efx_channel *channel; + +efx_for_each_channel (channel, efx) +irq_set_affinity_hint (channel->irq,NULL ); +} +#endif /* CONFIG_SMP */ +static int efx_soft_enable_interrupts (struct efx_nic *efx) {struct efx_channel *channel, *end_channel;
drivers/net/ethernet/sfc/efx.c static void efx_pci_remove_main(struct efx_nic *efx) · line 2961
Quoted 6 lines
drivers/net/ethernet/sfc/efx.c +3
-0@@ -2932,6 +2956,9 @@ static void efx_pci_remove_main(struct efx_nic *efx)cancel_work_sync (&efx->reset_work);efx_disable_interrupts (efx); +#if defined(CONFIG_SMP ) +efx_clear_interrupt_affinity (efx); +#endif
Please just define empty function for !SMP case instead of the ugly
#ifdef'fery in the functiojn bodies.
[...]
MBR, Sergei
Re: [PATCH net-next 2/4] sfc: allocate rx pages on the same node as the interrupt
In reply to Eric DumazetQuoted 92 lines · Eric Dumazet
Quoted 77 lines · Shradha Shah
From: Daniel Pieczko <[email protected]>
When the interrupt servicing a channel is on a NUMA node that is not local to the device, performance is improved by allocating rx pages on the node local to the interrupt (remote to the device)
The performance-optimal case, where interrupts and applications are pinned to CPUs on the same node as the device, is not altered by this change.
This change gave a 1% improvement in transaction rate using Nginx with all interrupts and Nginx threads on the node remote to the device. It also gave a small reduction in round-trip latency, again with the interrupt and application on a different node to the device.
Allocating rx pages based on the
channel->irq_nodevalue is only valid for the initial driver-load interrupt affinities; if an interrupt is moved later, the wrong node may be used for the allocation.Signed-off-by Shradha Shah <[email protected]>--- drivers/net/ethernet/sfc/efx.c | 1 + drivers/net/ethernet/sfc/net_driver.h | 3 +++ drivers/net/ethernet/sfc/rx.c | 14 +++++++++----- 3 files changed, 13 insertions(+), 5 deletions(-)drivers/net/ethernet/sfc/efx.c +1
-0@@ -445,6 +445,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel) channel->efx = efx; channel->channel = i; channel->type = &efx_default_channel_type; + channel->irq_node =NUMA_NO_NODE ;for (j =0 ; j <EFX_TXQ_TYPES ; j++) { tx_queue = &channel->tx_queue[j];drivers/net/ethernet/sfc/net_driver.h +3
-0@@ -419,6 +419,7 @@ enum efx_sync_events_state {* @sync_events_state: Current state of sync events on this channel * @sync_timestamp_major: Major part of the last ptp sync event * @sync_timestamp_minor: Minor part of the last ptp sync event +* @irq_node: NUMA node of interrupt */ struct efx_channel {struct efx_nic *efx;@@ -477,6 +478,8 @@ struct efx_channel {enum efx_sync_events_state sync_events_state;u32 sync_timestamp_major;u32 sync_timestamp_minor; + +int irq_node; };#ifdef CONFIG_NET_RX_BUSY_POLL drivers/net/ethernet/sfc/rx.c +2
-5@@ -168,11 +168,15 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)* context in such a case. So, use __GFP_NO_WARN * in case of atomic. */ - page =alloc_pages (__GFP_COLD | __GFP_COMP | - (atomic ? - (GFP_ATOMIC | __GFP_NOWARN) - :GFP_KERNEL ), - efx->rx_buffer_order); +struct efx_channel *channel; +drivers/net/ethernet/sfc/rx.c static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic) · line 182
Quoted 10 lines
drivers/net/ethernet/sfc/rx.c +7
-0+ channel =efx_rx_queue_channel (rx_queue); + page =alloc_pages_node (channel->irq_node, __GFP_COMP | + (atomic ? + (GFP_ATOMIC | __GFP_NOWARN) + :GFP_KERNEL ), + efx->rx_buffer_order); +if (unlikely (page ==NULL ))return -ENOMEM ; dma_addr =Sorry, I do not understand this patch, and why the following one is not squashed on this one.
Quoted 5 lines
irq_nodeis alwaysNUMA_NO_NODE(in this patch)So you claim a 1% improvement, switching from
alloc_pages(...)toalloc_pages_node(NUMA_NO_NODE, ...)???
You're correct that this doesn't make sense as it is. There is something missing in this patch (channel->irq_node should be set) and also changing the order of some patches could make this clearer. The series will need to be resent.
Daniel