| Message ID | 4FA822C4.60809@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 07/05/2012 21:30, Erwan Velu a écrit : > From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001 > From: Erwan Velu <erwan.velu@zodiacaerospace.com> > Date: Mon, 7 May 2012 19:15:29 +0000 > Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers Hey Fellows, Does my patch can be considered as acceptable or shall I rework something on it ? Cheers, Erwan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Erwan Velu <erwanaliasr1@gmail.com> Date: Tue, 08 May 2012 20:35:28 +0200 > Le 07/05/2012 21:30, Erwan Velu a écrit : >> From bb1e271db0fa1a29df19bede69faf8004389132d Mon Sep 17 00:00:00 2001 >> From: Erwan Velu <erwan.velu@zodiacaerospace.com> >> Date: Mon, 7 May 2012 19:15:29 +0000 >> Subject: [PATCH 1/1] pch_gbe: Adding read memory barriers > > Does my patch can be considered as acceptable or shall I rework > something on it ? You never need to ask questions like this. Your patch is queued up to be reviewed in patchwork: http://patchwork.ozlabs.org/project/netdev/list/ Therefore you only make more work for maintainers and irritate them by asking this, and therefore it will take even longer for them to get to your patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 08/05/2012 20:39, David Miller a écrit : > You never need to ask questions like this. Your patch is queued up to > be reviewed in patchwork: > http://patchwork.ozlabs.org/project/netdev/list/ Therefore you only > make more work for maintainers and irritate them by asking this, and > therefore it will take even longer for them to get to your patch. It wasn't my aim to irritate anyone. I'm just brand new into committing something here and surely lack of a good understanding on the complete process. I do understand you are very solicited and newbies like me can sometimes irritate by asking questions & doing thing not properly. Anyway, thanks for your help & patience. Erwan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Under a strong incoming packet stream and/or high cpu usage, > the pch_gbe driver reports "Receive CRC Error" and discards packet. > > It occurred on an Intel ATOM E620T while running a > 300mbit/sec multicast > network stream leading to a ~100% cpu usage. > > Adding rmb() calls before considering the network card's status solve > this issue. > > Getting it into stable would be perfect as it solves > reliability issues. > > Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com> > --- > .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > index 8035e5f..ace3654 100644 > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > @@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int > irq, void *data) > pch_gbe_mac_set_pause_packet(hw); > } > } > + smp_rmb(); /* prevent any other reads before*/ Under the assumption that your memory references are uncached, you only need to stop gcc reordering the object code, Rather than actually adding one of the 'fence' instructions. So you should only need: asm volatile(:::"memory") NFI which define generates that, the defines in the copy of sysdep.h I just looked at always include one of the fences. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-05-09 at 11:47 +0100, David Laight wrote: > > Under a strong incoming packet stream and/or high cpu usage, > > the pch_gbe driver reports "Receive CRC Error" and discards packet. > > > > It occurred on an Intel ATOM E620T while running a > > 300mbit/sec multicast > > network stream leading to a ~100% cpu usage. > > > > Adding rmb() calls before considering the network card's status solve > > this issue. > > > > Getting it into stable would be perfect as it solves > > reliability issues. > > > > Signed-off-by: Erwan Velu <erwan.velu@zodiacaerospace.com> > > --- > > .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > > b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > > index 8035e5f..ace3654 100644 > > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c > > @@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int > > irq, void *data) > > pch_gbe_mac_set_pause_packet(hw); > > } > > } > > + smp_rmb(); /* prevent any other reads before*/ > > Under the assumption that your memory references are uncached, > you only need to stop gcc reordering the object code, > Rather than actually adding one of the 'fence' instructions. Also, the usual MMIO functions already include compiler barriers. > So you should only need: asm volatile(:::"memory") > NFI which define generates that, the defines in the copy of > sysdep.h I just looked at always include one of the fences. This is barrier(). But I think this must be intended to control ordering of fields that are written elsewhere. Really, this needs a much more specific comment to explain the intent. Then reviewers can work out whether it actually achieves the intent! Ben.
On Wed, 2012-05-09 at 17:20 +0200, Erwan Velu wrote: > Thanks for this very clear description. It might be clear, but it's not very relevant: you need to understand the Linux kernel barrier APIs (which cover all architectures). See Documentation/memory-barriers.txt. > As I'm pretty new to this kind of integration in the kernel, do you > request me to test this volatile option ? > Does the patch tends to be rejected for this issue ? > What should be the next step for my patch ? [...] Explain what kind of reordering you are trying to avoid. Ben.
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c index 8035e5f..ace3654 100644 --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c @@ -1413,6 +1413,7 @@ static irqreturn_t pch_gbe_intr(int irq, void *data) pch_gbe_mac_set_pause_packet(hw); } } + smp_rmb(); /* prevent any other reads before*/ /* When request status is Receive interruption */ if ((int_st & (PCH_GBE_INT_RX_DMA_CMPLT | PCH_GBE_INT_TX_CMPLT)) || @@ -1582,6 +1584,7 @@ pch_gbe_clean_tx(struct pch_gbe_adapter *adapter, i = tx_ring->next_to_clean; tx_desc = PCH_GBE_TX_DESC(*tx_ring, i); + rmb(); /* prevent any other reads before*/ pr_debug("gbec_status:0x%04x dma_status:0x%04x\n", tx_desc->gbec_status, tx_desc->dma_status); @@ -1682,6 +1685,7 @@ pch_gbe_clean_rx(struct pch_gbe_adapter *adapter, while (*work_done < work_to_do) { /* Check Rx descriptor status */ rx_desc = PCH_GBE_RX_DESC(*rx_ring, i); + rmb(); /* prevent any other reads before*/ if (rx_desc->gbec_status == DSC_INIT16) break; cleaned = true;