* [PATCH 0/2] net: AWS ENA: Fix memory barrier usage when using LLQ @ 2020-01-02 18:08 Liran Alon 2020-01-02 18:08 ` [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer Liran Alon 2020-01-02 18:08 ` [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell Liran Alon 0 siblings, 2 replies; 9+ messages in thread From: Liran Alon @ 2020-01-02 18:08 UTC (permalink / raw) To: netanel, davem, netdev Cc: saeedb, zorik, sameehj, igorch, akiyano, evgenys, gtzalik, ndagan, matua, galpress Hi, This simple patch-series address 2 issues found during code-review of AWS ENA NIC driver related to how it use memory barriers when NIC is running in "low-latency queue" mode (LLQ). 1st patch removes an unnecessary wmb(). 2nd patch fix a bug of not flushing write-combined buffers holding LLQ transmit descriptors and first packet bytes before writing new SQ tail to device. This bug is introduced because of a weird behaviour of x86 AMD CPU that it doesn't flush write-combined buffers when CPU writes to UC memory as x86 Intel CPU does. Which makes writeX() insufficient in order to guarantee write-combined buffers are flushed. This patch makes sure to just fix AWS ENA to handle this case properly, but a future patch-series should be submitted to probably introduce a new flush_wc_writeX() macro that handles this case properly for all CPU archs and vendors. For more info, see patch commit message itself. Regards, -Liran ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer 2020-01-02 18:08 [PATCH 0/2] net: AWS ENA: Fix memory barrier usage when using LLQ Liran Alon @ 2020-01-02 18:08 ` Liran Alon 2020-01-17 15:54 ` Liran Alon 2020-01-02 18:08 ` [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell Liran Alon 1 sibling, 1 reply; 9+ messages in thread From: Liran Alon @ 2020-01-02 18:08 UTC (permalink / raw) To: netanel, davem, netdev Cc: saeedb, zorik, sameehj, igorch, akiyano, evgenys, gtzalik, ndagan, matua, galpress, Liran Alon, Håkon Bugge Current code executes wmb() in order to flush writes to bounce buffer before copying it to device-memory (PCI BAR mapped as WC) to ensure consistent data is written to device-memory. However, this wmb() is unnecessary. This is because all reads from the buffer are guaranteed to be consistent with previous writes to the buffer done from the same task (Which is the only one that writes to the buffer). i.e. If a single CPU runs both the writes to the buffer and the reads from the buffer, the reads are guaranteed to read most up-to-date data in program order (E.g. Due to store-to-load-forwarding mechanism). Otherwise, there is a context-switch, and that should make writes before context-switch globally visible as-well. Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- drivers/net/ethernet/amazon/ena/ena_eth_com.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.c b/drivers/net/ethernet/amazon/ena/ena_eth_com.c index 2845ac277724..742578ac1240 100644 --- a/drivers/net/ethernet/amazon/ena/ena_eth_com.c +++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.c @@ -93,11 +93,6 @@ static int ena_com_write_bounce_buffer_to_dev(struct ena_com_io_sq *io_sq, io_sq->qid, io_sq->entries_in_tx_burst_left); } - /* Make sure everything was written into the bounce buffer before - * writing the bounce buffer to the device - */ - wmb(); - /* The line is completed. Copy it to dev */ __iowrite64_copy(io_sq->desc_addr.pbuf_dev_addr + dst_offset, bounce_buffer, (llq_info->desc_list_entry_size) / 8); -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer 2020-01-02 18:08 ` [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer Liran Alon @ 2020-01-17 15:54 ` Liran Alon 0 siblings, 0 replies; 9+ messages in thread From: Liran Alon @ 2020-01-17 15:54 UTC (permalink / raw) To: Liran Alon Cc: Belgazal, Netanel, davem, netdev, saeedb, zorik, sameehj, igorch, akiyano, evgenys, gtzalik, ndagan, matua, galpress, Håkon Bugge Ping on merging this patch (Patch 2/2 of series should be dropped). -Liran > On 2 Jan 2020, at 20:08, Liran Alon <liran.alon@oracle.com> wrote: > > Current code executes wmb() in order to flush writes to bounce buffer > before copying it to device-memory (PCI BAR mapped as WC) to ensure > consistent data is written to device-memory. > > However, this wmb() is unnecessary. This is because all reads from the > buffer are guaranteed to be consistent with previous writes to the buffer > done from the same task (Which is the only one that writes to the buffer). > > i.e. If a single CPU runs both the writes to the buffer and the reads > from the buffer, the reads are guaranteed to read most up-to-date data > in program order (E.g. Due to store-to-load-forwarding mechanism). > Otherwise, there is a context-switch, and that should make writes before > context-switch globally visible as-well. > > Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > drivers/net/ethernet/amazon/ena/ena_eth_com.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.c b/drivers/net/ethernet/amazon/ena/ena_eth_com.c > index 2845ac277724..742578ac1240 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_eth_com.c > +++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.c > @@ -93,11 +93,6 @@ static int ena_com_write_bounce_buffer_to_dev(struct ena_com_io_sq *io_sq, > io_sq->qid, io_sq->entries_in_tx_burst_left); > } > > - /* Make sure everything was written into the bounce buffer before > - * writing the bounce buffer to the device > - */ > - wmb(); > - > /* The line is completed. Copy it to dev */ > __iowrite64_copy(io_sq->desc_addr.pbuf_dev_addr + dst_offset, > bounce_buffer, (llq_info->desc_list_entry_size) / 8); > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-02 18:08 [PATCH 0/2] net: AWS ENA: Fix memory barrier usage when using LLQ Liran Alon 2020-01-02 18:08 ` [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer Liran Alon @ 2020-01-02 18:08 ` Liran Alon 2020-01-03 18:46 ` Liran Alon 1 sibling, 1 reply; 9+ messages in thread From: Liran Alon @ 2020-01-02 18:08 UTC (permalink / raw) To: netanel, davem, netdev Cc: saeedb, zorik, sameehj, igorch, akiyano, evgenys, gtzalik, ndagan, matua, galpress, Liran Alon, Håkon Bugge AWS ENA NIC supports Tx SQ in Low Latency Queue (LLQ) mode (Also referred to as "push-mode"). In this mode, the driver pushes the transmit descriptors and the first 128 bytes of the packet directly to the ENA device memory space, while the rest of the packet payload is fetched by the device from host memory. For this operation mode, the driver uses a dedicated PCI BAR which is mapped as WC memory. The function ena_com_write_bounce_buffer_to_dev() is responsible to write to the above mentioned PCI BAR. When the write of new SQ tail to doorbell is visible to device, device expects to be able to read relevant transmit descriptors and packets headers from device memory. Therefore, driver should ensure write-combined buffers (WCBs) are flushed before the write to doorbell is visible to the device. For some CPUs, this will be taken care of by writel(). For example, x86 Intel CPUs flushes write-combined buffers when a read or write is done to UC memory (In our case, the doorbell). See Intel SDM section 11.3 METHODS OF CACHING AVAILABLE: "If the WC buffer is partially filled, the writes may be delayed until the next occurrence of a serializing event; such as, an SFENCE or MFENCE instruction, CPUID execution, a read or write to uncached memory, an interrupt occurrence, or a LOCK instruction execution.” However, other CPUs do not provide this guarantee. For example, x86 AMD CPUs flush write-combined buffers only on a read from UC memory. Not a write to UC memory. See AMD Software Optimisation Guide for AMD Family 17h Processors section 2.13.3 Write-Combining Operations. Therefore, modify ena_com_write_sq_doorbell() to flush write-combined buffers with wmb() in case Tx SQ is in LLQ mode. Note that this cause 2 theoretical unnecessary perf hits: (1) On x86 Intel, this will execute unnecessary SFENCE. But probably the perf impact is neglictable because it will also cause the implciit SFENCE done internally by write to UC memory to do less work. (2) On ARM64 this will change from using dma_wmb() to using wmb() which is more costly (Use DSB instead of DMB) even though DMB should be sufficient to flush WCBs. This patch will focus on making sure WCBs are flushed on all CPUs, and a later future patch will be made to add a new macro to Linux such as flush_wc_writeX() that does the right thing for all archs and CPU vendors. Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- drivers/net/ethernet/amazon/ena/ena_eth_com.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.h b/drivers/net/ethernet/amazon/ena/ena_eth_com.h index 77986c0ea52c..f9bfaef08bfa 100644 --- a/drivers/net/ethernet/amazon/ena/ena_eth_com.h +++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.h @@ -179,7 +179,22 @@ static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq) pr_debug("write submission queue doorbell for queue: %d tail: %d\n", io_sq->qid, tail); - writel(tail, io_sq->db_addr); + /* + * When Tx SQ is in LLQ mode, transmit descriptors and packet headers + * are written to device-memory mapped as WC. Therefore, we need to + * ensure write-combined buffers are flushed before writing new SQ + * tail to doorbell. + * + * On some CPUs (E.g. x86 AMD) writel() doesn't guarantee this. + * Therefore, prefer to explicitly flush write-combined buffers + * with wmb() before writing to doorbell in case Tx SQ is in LLQ mode. + */ + if (io_sq->mem_queue_type == ENA_ADMIN_PLACEMENT_POLICY_DEV) { + wmb(); + writel_relaxed(tail, io_sq->db_addr); + } else { + writel(tail, io_sq->db_addr); + } if (is_llq_max_tx_burst_exists(io_sq)) { pr_debug("reset available entries in tx burst for queue %d to %d\n", -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-02 18:08 ` [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell Liran Alon @ 2020-01-03 18:46 ` Liran Alon 2020-01-04 4:55 ` Machulsky, Zorik 0 siblings, 1 reply; 9+ messages in thread From: Liran Alon @ 2020-01-03 18:46 UTC (permalink / raw) To: Liran Alon Cc: Belgazal, Netanel, davem, netdev, saeedb, zorik, sameehj, igorch, akiyano, evgenys, gtzalik, ndagan, matua, galpress, Håkon Bugge > On 2 Jan 2020, at 20:08, Liran Alon <liran.alon@oracle.com> wrote: > > AWS ENA NIC supports Tx SQ in Low Latency Queue (LLQ) mode (Also > referred to as "push-mode"). In this mode, the driver pushes the > transmit descriptors and the first 128 bytes of the packet directly > to the ENA device memory space, while the rest of the packet payload > is fetched by the device from host memory. For this operation mode, > the driver uses a dedicated PCI BAR which is mapped as WC memory. > > The function ena_com_write_bounce_buffer_to_dev() is responsible > to write to the above mentioned PCI BAR. > > When the write of new SQ tail to doorbell is visible to device, device > expects to be able to read relevant transmit descriptors and packets > headers from device memory. Therefore, driver should ensure > write-combined buffers (WCBs) are flushed before the write to doorbell > is visible to the device. > > For some CPUs, this will be taken care of by writel(). For example, > x86 Intel CPUs flushes write-combined buffers when a read or write > is done to UC memory (In our case, the doorbell). See Intel SDM section > 11.3 METHODS OF CACHING AVAILABLE: > "If the WC buffer is partially filled, the writes may be delayed until > the next occurrence of a serializing event; such as, an SFENCE or MFENCE > instruction, CPUID execution, a read or write to uncached memory, an > interrupt occurrence, or a LOCK instruction execution.” > > However, other CPUs do not provide this guarantee. For example, x86 > AMD CPUs flush write-combined buffers only on a read from UC memory. > Not a write to UC memory. See AMD Software Optimisation Guide for AMD > Family 17h Processors section 2.13.3 Write-Combining Operations. Actually... After re-reading AMD Optimization Guide SDM, I see it is guaranteed that: “Write-combining is closed if all 64 bytes of the write buffer are valid”. And this is indeed always the case for AWS ENA LLQ. Because as can be seen at ena_com_config_llq_info(), desc_list_entry_size is either 128, 192 or 256. i.e. Always a multiple of 64 bytes. So this patch in theory could maybe be dropped as for x86 Intel & AMD and ARM64 with current desc_list_entry_size, it isn’t strictly necessary to guarantee that WC buffers are flushed. I will let AWS folks to decide if they prefer to apply this patch anyway to make WC flush explicit and to avoid hard-to-debug issues in case of new non-64-multiply size appear in the future. Or to drop this patch and instead add a WARN_ON() to ena_com_config_llq_info() in case desc_list_entry_size is not a multiple of 64 bytes. To avoid taking perf hit for no real value. -Liran ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-03 18:46 ` Liran Alon @ 2020-01-04 4:55 ` Machulsky, Zorik 2020-01-05 9:53 ` Bshara, Saeed 0 siblings, 1 reply; 9+ messages in thread From: Machulsky, Zorik @ 2020-01-04 4:55 UTC (permalink / raw) To: Liran Alon Cc: Belgazal, Netanel, davem, netdev, Bshara, Saeed, Jubran, Samih, Chauskin, Igor, Kiyanovski, Arthur, Schmeilin, Evgeny, Tzalik, Guy, Dagan, Noam, Matushevsky, Alexander, Pressman, Gal, Håkon Bugge On 1/3/20, 1:47 PM, "Liran Alon" <liran.alon@oracle.com> wrote: > On 2 Jan 2020, at 20:08, Liran Alon <liran.alon@oracle.com> wrote: > > AWS ENA NIC supports Tx SQ in Low Latency Queue (LLQ) mode (Also > referred to as "push-mode"). In this mode, the driver pushes the > transmit descriptors and the first 128 bytes of the packet directly > to the ENA device memory space, while the rest of the packet payload > is fetched by the device from host memory. For this operation mode, > the driver uses a dedicated PCI BAR which is mapped as WC memory. > > The function ena_com_write_bounce_buffer_to_dev() is responsible > to write to the above mentioned PCI BAR. > > When the write of new SQ tail to doorbell is visible to device, device > expects to be able to read relevant transmit descriptors and packets > headers from device memory. Therefore, driver should ensure > write-combined buffers (WCBs) are flushed before the write to doorbell > is visible to the device. > > For some CPUs, this will be taken care of by writel(). For example, > x86 Intel CPUs flushes write-combined buffers when a read or write > is done to UC memory (In our case, the doorbell). See Intel SDM section > 11.3 METHODS OF CACHING AVAILABLE: > "If the WC buffer is partially filled, the writes may be delayed until > the next occurrence of a serializing event; such as, an SFENCE or MFENCE > instruction, CPUID execution, a read or write to uncached memory, an > interrupt occurrence, or a LOCK instruction execution.” > > However, other CPUs do not provide this guarantee. For example, x86 > AMD CPUs flush write-combined buffers only on a read from UC memory. > Not a write to UC memory. See AMD Software Optimisation Guide for AMD > Family 17h Processors section 2.13.3 Write-Combining Operations. Actually... After re-reading AMD Optimization Guide SDM, I see it is guaranteed that: “Write-combining is closed if all 64 bytes of the write buffer are valid”. And this is indeed always the case for AWS ENA LLQ. Because as can be seen at ena_com_config_llq_info(), desc_list_entry_size is either 128, 192 or 256. i.e. Always a multiple of 64 bytes. So this patch in theory could maybe be dropped as for x86 Intel & AMD and ARM64 with current desc_list_entry_size, it isn’t strictly necessary to guarantee that WC buffers are flushed. I will let AWS folks to decide if they prefer to apply this patch anyway to make WC flush explicit and to avoid hard-to-debug issues in case of new non-64-multiply size appear in the future. Or to drop this patch and instead add a WARN_ON() to ena_com_config_llq_info() in case desc_list_entry_size is not a multiple of 64 bytes. To avoid taking perf hit for no real value. Liran, thanks for this important info. If this is the case, I believe we should drop this patch as it introduces unnecessary branch in data path. Agree with your WARN_ON() suggestion. -Liran ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-04 4:55 ` Machulsky, Zorik @ 2020-01-05 9:53 ` Bshara, Saeed 2020-01-05 10:22 ` Liran Alon 0 siblings, 1 reply; 9+ messages in thread From: Bshara, Saeed @ 2020-01-05 9:53 UTC (permalink / raw) To: Liran Alon, Machulsky, Zorik Cc: Belgazal, Netanel, davem, netdev, Jubran, Samih, Chauskin, Igor, Kiyanovski, Arthur, Schmeilin, Evgeny, Tzalik, Guy, Dagan, Noam, Matushevsky, Alexander, Pressman, Gal, Håkon Bugge Thanks Liran, I think we missed the payload visibility; The LLQ descriptor contains the header part of the packet, in theory we will need also to make sure that all cpu writes to the packet payload are visible to the device, I bet that in practice those stores will be visible without explicit barrier, but we better stick to the rules. so we still need dma_wmb(), also, that means the first patch can't simply remove the wmb() as it actually may be taking care for the payload visibility. saeed From: Machulsky, Zorik Sent: Saturday, January 4, 2020 6:55 AM To: Liran Alon Cc: Belgazal, Netanel; davem@davemloft.net; netdev@vger.kernel.org; Bshara, Saeed; Jubran, Samih; Chauskin, Igor; Kiyanovski, Arthur; Schmeilin, Evgeny; Tzalik, Guy; Dagan, Noam; Matushevsky, Alexander; Pressman, Gal; Håkon Bugge Subject: Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell On 1/3/20, 1:47 PM, "Liran Alon" <liran.alon@oracle.com> wrote: > On 2 Jan 2020, at 20:08, Liran Alon <liran.alon@oracle.com> wrote: > > AWS ENA NIC supports Tx SQ in Low Latency Queue (LLQ) mode (Also > referred to as "push-mode"). In this mode, the driver pushes the > transmit descriptors and the first 128 bytes of the packet directly > to the ENA device memory space, while the rest of the packet payload > is fetched by the device from host memory. For this operation mode, > the driver uses a dedicated PCI BAR which is mapped as WC memory. > > The function ena_com_write_bounce_buffer_to_dev() is responsible > to write to the above mentioned PCI BAR. > > When the write of new SQ tail to doorbell is visible to device, device > expects to be able to read relevant transmit descriptors and packets > headers from device memory. Therefore, driver should ensure > write-combined buffers (WCBs) are flushed before the write to doorbell > is visible to the device. > > For some CPUs, this will be taken care of by writel(). For example, > x86 Intel CPUs flushes write-combined buffers when a read or write > is done to UC memory (In our case, the doorbell). See Intel SDM section > 11.3 METHODS OF CACHING AVAILABLE: > "If the WC buffer is partially filled, the writes may be delayed until > the next occurrence of a serializing event; such as, an SFENCE or MFENCE > instruction, CPUID execution, a read or write to uncached memory, an > interrupt occurrence, or a LOCK instruction execution.” > > However, other CPUs do not provide this guarantee. For example, x86 > AMD CPUs flush write-combined buffers only on a read from UC memory. > Not a write to UC memory. See AMD Software Optimisation Guide for AMD > Family 17h Processors section 2.13.3 Write-Combining Operations. Actually... After re-reading AMD Optimization Guide SDM, I see it is guaranteed that: “Write-combining is closed if all 64 bytes of the write buffer are valid”. And this is indeed always the case for AWS ENA LLQ. Because as can be seen at ena_com_config_llq_info(), desc_list_entry_size is either 128, 192 or 256. i.e. Always a multiple of 64 bytes. So this patch in theory could maybe be dropped as for x86 Intel & AMD and ARM64 with current desc_list_entry_size, it isn’t strictly necessary to guarantee that WC buffers are flushed. I will let AWS folks to decide if they prefer to apply this patch anyway to make WC flush explicit and to avoid hard-to-debug issues in case of new non-64-multiply size appear in the future. Or to drop this patch and instead add a WARN_ON() to ena_com_config_llq_info() in case desc_list_entry_size is not a multiple of 64 bytes. To avoid taking perf hit for no real value. Liran, thanks for this important info. If this is the case, I believe we should drop this patch as it introduces unnecessary branch in data path. Agree with your WARN_ON() suggestion. -Liran ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-05 9:53 ` Bshara, Saeed @ 2020-01-05 10:22 ` Liran Alon 2020-01-05 11:49 ` Bshara, Saeed 0 siblings, 1 reply; 9+ messages in thread From: Liran Alon @ 2020-01-05 10:22 UTC (permalink / raw) To: Bshara, Saeed Cc: Machulsky, Zorik, Belgazal, Netanel, davem, netdev, Jubran, Samih, Chauskin, Igor, Kiyanovski, Arthur, Schmeilin, Evgeny, Tzalik, Guy, Dagan, Noam, Matushevsky, Alexander, Pressman, Gal, Håkon Bugge Hi Saeed, If I understand correctly, the device is only aware of new descriptors once the tail is updated by ena_com_write_sq_doorbell() using writel(). If that’s the case, then writel() guarantees all previous writes to WB/UC memory is visible to device before the write done by writel(). If device is allowed to fetch packet payload at the moment the transmit descriptor is written into device-memory using LLQ, then ena_com_write_bounce_buffer_to_dev() should dma_wmb() before __iowrite64_copy(). Instead of wmb(). And comment is wrong and should be updated accordingly. For example, this will optimise x86 to only have a compiler-barrier instead of executing a SFENCE. Can you clarify what is device behaviour on when it is allowed to read the packet payload? i.e. Is it only after writing to doorbell or is it from the moment the transmit descriptor is written to LLQ? -Liran > On 5 Jan 2020, at 11:53, Bshara, Saeed <saeedb@amazon.com> wrote: > > > Thanks Liran, > > I think we missed the payload visibility; The LLQ descriptor contains the header part of the packet, in theory we will need also to make sure that all cpu writes to the packet payload are visible to the device, I bet that in practice those stores will be visible without explicit barrier, but we better stick to the rules. > so we still need dma_wmb(), also, that means the first patch can't simply remove the wmb() as it actually may be taking care for the payload visibility. > > saeed > > From: Machulsky, Zorik > Sent: Saturday, January 4, 2020 6:55 AM > To: Liran Alon > Cc: Belgazal, Netanel; davem@davemloft.net; netdev@vger.kernel.org; Bshara, Saeed; Jubran, Samih; Chauskin, Igor; Kiyanovski, Arthur; Schmeilin, Evgeny; Tzalik, Guy; Dagan, Noam; Matushevsky, Alexander; Pressman, Gal; Håkon Bugge > Subject: Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell > > > > On 1/3/20, 1:47 PM, "Liran Alon" <liran.alon@oracle.com> wrote: > > > > > On 2 Jan 2020, at 20:08, Liran Alon <liran.alon@oracle.com> wrote: > > > > AWS ENA NIC supports Tx SQ in Low Latency Queue (LLQ) mode (Also > > referred to as "push-mode"). In this mode, the driver pushes the > > transmit descriptors and the first 128 bytes of the packet directly > > to the ENA device memory space, while the rest of the packet payload > > is fetched by the device from host memory. For this operation mode, > > the driver uses a dedicated PCI BAR which is mapped as WC memory. > > > > The function ena_com_write_bounce_buffer_to_dev() is responsible > > to write to the above mentioned PCI BAR. > > > > When the write of new SQ tail to doorbell is visible to device, device > > expects to be able to read relevant transmit descriptors and packets > > headers from device memory. Therefore, driver should ensure > > write-combined buffers (WCBs) are flushed before the write to doorbell > > is visible to the device. > > > > For some CPUs, this will be taken care of by writel(). For example, > > x86 Intel CPUs flushes write-combined buffers when a read or write > > is done to UC memory (In our case, the doorbell). See Intel SDM section > > 11.3 METHODS OF CACHING AVAILABLE: > > "If the WC buffer is partially filled, the writes may be delayed until > > the next occurrence of a serializing event; such as, an SFENCE or MFENCE > > instruction, CPUID execution, a read or write to uncached memory, an > > interrupt occurrence, or a LOCK instruction execution.” > > > > However, other CPUs do not provide this guarantee. For example, x86 > > AMD CPUs flush write-combined buffers only on a read from UC memory. > > Not a write to UC memory. See AMD Software Optimisation Guide for AMD > > Family 17h Processors section 2.13.3 Write-Combining Operations. > > Actually... After re-reading AMD Optimization Guide SDM, I see it is guaranteed that: > “Write-combining is closed if all 64 bytes of the write buffer are valid”. > And this is indeed always the case for AWS ENA LLQ. Because as can be seen at > ena_com_config_llq_info(), desc_list_entry_size is either 128, 192 or 256. i.e. Always > a multiple of 64 bytes. > > So this patch in theory could maybe be dropped as for x86 Intel & AMD and ARM64 with > current desc_list_entry_size, it isn’t strictly necessary to guarantee that WC buffers are flushed. > > I will let AWS folks to decide if they prefer to apply this patch anyway to make WC flush explicit > and to avoid hard-to-debug issues in case of new non-64-multiply size appear in the future. Or > to drop this patch and instead add a WARN_ON() to ena_com_config_llq_info() in case desc_list_entry_size > is not a multiple of 64 bytes. To avoid taking perf hit for no real value. > > Liran, thanks for this important info. If this is the case, I believe we should drop this patch as it introduces unnecessary branch > in data path. Agree with your WARN_ON() suggestion. > > -Liran > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell 2020-01-05 10:22 ` Liran Alon @ 2020-01-05 11:49 ` Bshara, Saeed 0 siblings, 0 replies; 9+ messages in thread From: Bshara, Saeed @ 2020-01-05 11:49 UTC (permalink / raw) To: liran.alon Cc: Kiyanovski, Arthur, Tzalik, Guy, Matushevsky, Alexander, Jubran, Samih, haakon.bugge, Pressman, Gal, Dagan, Noam, Machulsky, Zorik, Schmeilin, Evgeny, netdev, davem, Belgazal, Netanel, Chauskin, Igor On Sun, 2020-01-05 at 12:22 +0200, Liran Alon wrote: > Hi Saeed, > > If I understand correctly, the device is only aware of new > descriptors once the tail is updated by ena_com_write_sq_doorbell() > using writel(). > If that’s the case, then writel() guarantees all previous writes to > WB/UC memory is visible to device before the write done by writel(). device fetches packet only after doorbell notification. you are right, writel() includes the needed barrier ( https://elixir.bootlin.com/linux/v5.4.8/source/Documentation/memory-barriers.txt#L1929 ) so indeed we should be ok without any explicit wmb() or dma_wmb(). > > If device is allowed to fetch packet payload at the moment the > transmit descriptor is written into device-memory using LLQ, > then ena_com_write_bounce_buffer_to_dev() should dma_wmb() before > __iowrite64_copy(). Instead of wmb(). And comment > is wrong and should be updated accordingly. > For example, this will optimise x86 to only have a compiler-barrier > instead of executing a SFENCE. > > Can you clarify what is device behaviour on when it is allowed to > read the packet payload? > i.e. Is it only after writing to doorbell or is it from the moment > the transmit descriptor is written to LLQ? > > -Liran > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-17 15:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-02 18:08 [PATCH 0/2] net: AWS ENA: Fix memory barrier usage when using LLQ Liran Alon 2020-01-02 18:08 ` [PATCH 1/2] net: AWS ENA: Remove unncessary wmb() to flush bounce buffer Liran Alon 2020-01-17 15:54 ` Liran Alon 2020-01-02 18:08 ` [PATCH 2/2] net: AWS ENA: Flush WCBs before writing new SQ tail to doorbell Liran Alon 2020-01-03 18:46 ` Liran Alon 2020-01-04 4:55 ` Machulsky, Zorik 2020-01-05 9:53 ` Bshara, Saeed 2020-01-05 10:22 ` Liran Alon 2020-01-05 11:49 ` Bshara, Saeed
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.