From: Alexander Duyck <alexander.duyck@gmail.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Netdev <netdev@vger.kernel.org>,
oss-drivers@netronome.com, Jakub Kicinski <kubakici@wp.pl>
Subject: Re: [PATCH net-next 1/5] nfp: make use of the DMA_ATTR_SKIP_CPU_SYNC attr
Date: Fri, 21 Apr 2017 08:07:45 -0700 [thread overview]
Message-ID: <CAKgT0UfdbLV9tdYZXVQK6U2cF1Ljzc27uuVoNaZF-rHcmLUnPQ@mail.gmail.com> (raw)
In-Reply-To: <20170421142052.107388-2-jakub.kicinski@netronome.com>
On Fri, Apr 21, 2017 at 7:20 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> DMA unmap may destroy changes CPU made to the buffer. To make XDP
> run correctly on non-x86 platforms we should use the
> DMA_ATTR_SKIP_CPU_SYNC attribute.
>
> Thanks to using the attribute we can now push the sync operation to
> the common code path from XDP handler.
>
> A little bit of variable name reshuffling is required to bring the
> code back to readable state.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
So I see where you added the sync_single_for_cpu, but what about the
sync_single_for_device? It needs to be called for the buffer before
you assign it for Rx. On x86 it won't really matter but for proper
utilization of the DMA API you need to sync the buffer even for Rx
just to make certain that the cache lines are evicted prior to the
device attempting to write to the buffer.
> ---
> .../net/ethernet/netronome/nfp/nfp_net_common.c | 43 +++++++++++++---------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index e2197160e4dc..1274a70c9a38 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -87,16 +87,23 @@ void nfp_net_get_fw_version(struct nfp_net_fw_version *fw_ver,
>
> static dma_addr_t nfp_net_dma_map_rx(struct nfp_net_dp *dp, void *frag)
> {
> - return dma_map_single(dp->dev, frag + NFP_NET_RX_BUF_HEADROOM,
> - dp->fl_bufsz - NFP_NET_RX_BUF_NON_DATA,
> - dp->rx_dma_dir);
> + return dma_map_single_attrs(dp->dev, frag + NFP_NET_RX_BUF_HEADROOM,
> + dp->fl_bufsz - NFP_NET_RX_BUF_NON_DATA,
> + dp->rx_dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> }
>
> static void nfp_net_dma_unmap_rx(struct nfp_net_dp *dp, dma_addr_t dma_addr)
> {
> - dma_unmap_single(dp->dev, dma_addr,
> - dp->fl_bufsz - NFP_NET_RX_BUF_NON_DATA,
> - dp->rx_dma_dir);
> + dma_unmap_single_attrs(dp->dev, dma_addr,
> + dp->fl_bufsz - NFP_NET_RX_BUF_NON_DATA,
> + dp->rx_dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> +}
> +
> +static void nfp_net_dma_sync_cpu_rx(struct nfp_net_dp *dp, dma_addr_t dma_addr,
> + unsigned int len)
> +{
> + dma_sync_single_for_cpu(dp->dev, dma_addr - NFP_NET_RX_BUF_HEADROOM,
> + len, dp->rx_dma_dir);
> }
>
> /* Firmware reconfig
> @@ -1569,7 +1576,7 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> tx_ring = r_vec->xdp_ring;
>
> while (pkts_polled < budget) {
> - unsigned int meta_len, data_len, data_off, pkt_len;
> + unsigned int meta_len, data_len, meta_off, pkt_len, pkt_off;
> u8 meta_prepend[NFP_NET_MAX_PREPEND];
> struct nfp_net_rx_buf *rxbuf;
> struct nfp_net_rx_desc *rxd;
> @@ -1608,11 +1615,12 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> data_len = le16_to_cpu(rxd->rxd.data_len);
> pkt_len = data_len - meta_len;
>
> + pkt_off = NFP_NET_RX_BUF_HEADROOM + dp->rx_dma_off;
> if (dp->rx_offset == NFP_NET_CFG_RX_OFFSET_DYNAMIC)
> - data_off = NFP_NET_RX_BUF_HEADROOM + meta_len;
> + pkt_off += meta_len;
> else
> - data_off = NFP_NET_RX_BUF_HEADROOM + dp->rx_offset;
> - data_off += dp->rx_dma_off;
> + pkt_off += dp->rx_offset;
> + meta_off = pkt_off - meta_len;
>
> /* Stats update */
> u64_stats_update_begin(&r_vec->rx_sync);
> @@ -1621,7 +1629,7 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> u64_stats_update_end(&r_vec->rx_sync);
>
> /* Pointer to start of metadata */
> - meta = rxbuf->frag + data_off - meta_len;
> + meta = rxbuf->frag + meta_off;
>
> if (unlikely(meta_len > NFP_NET_MAX_PREPEND ||
> (dp->rx_offset && meta_len > dp->rx_offset))) {
> @@ -1631,6 +1639,9 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> continue;
> }
>
> + nfp_net_dma_sync_cpu_rx(dp, rxbuf->dma_addr + meta_off,
> + data_len);
> +
> if (xdp_prog && !(rxd->rxd.flags & PCIE_DESC_RX_BPF &&
> dp->bpf_offload_xdp)) {
> unsigned int dma_off;
> @@ -1638,10 +1649,6 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> int act;
>
> hard_start = rxbuf->frag + NFP_NET_RX_BUF_HEADROOM;
> - dma_off = data_off - NFP_NET_RX_BUF_HEADROOM;
> - dma_sync_single_for_cpu(dp->dev, rxbuf->dma_addr,
> - dma_off + pkt_len,
> - DMA_BIDIRECTIONAL);
>
> /* Move prepend out of the way */
> if (xdp_prog->xdp_adjust_head) {
> @@ -1650,12 +1657,12 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
> }
>
> act = nfp_net_run_xdp(xdp_prog, rxbuf->frag, hard_start,
> - &data_off, &pkt_len);
> + &pkt_off, &pkt_len);
> switch (act) {
> case XDP_PASS:
> break;
> case XDP_TX:
> - dma_off = data_off - NFP_NET_RX_BUF_HEADROOM;
> + dma_off = pkt_off - NFP_NET_RX_BUF_HEADROOM;
> if (unlikely(!nfp_net_tx_xdp_buf(dp, rx_ring,
> tx_ring, rxbuf,
> dma_off,
> @@ -1689,7 +1696,7 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget)
>
> nfp_net_rx_give_one(dp, rx_ring, new_frag, new_dma_addr);
>
> - skb_reserve(skb, data_off);
> + skb_reserve(skb, pkt_off);
> skb_put(skb, pkt_len);
>
> if (!dp->chained_metadata_format) {
> --
> 2.11.0
>
next prev parent reply other threads:[~2017-04-21 18:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-21 14:20 [PATCH net-next 0/5] nfp: DMA flags, adjust head and fixes Jakub Kicinski
2017-04-21 14:20 ` [PATCH net-next 1/5] nfp: make use of the DMA_ATTR_SKIP_CPU_SYNC attr Jakub Kicinski
2017-04-21 15:07 ` Alexander Duyck [this message]
2017-04-21 17:10 ` Jakub Kicinski
2017-04-21 19:37 ` David Miller
2017-04-21 14:20 ` [PATCH net-next 2/5] nfp: parse metadata prepend before XDP runs Jakub Kicinski
2017-04-21 14:20 ` [PATCH net-next 3/5] nfp: add NSP routine to get static information Jakub Kicinski
2017-04-21 14:20 ` [PATCH net-next 4/5] nfp: fix free list buffer size reporting Jakub Kicinski
2017-04-21 14:20 ` [PATCH net-next 5/5] nfp: remove the refresh of all ports optimization Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKgT0UfdbLV9tdYZXVQK6U2cF1Ljzc27uuVoNaZF-rHcmLUnPQ@mail.gmail.com \
--to=alexander.duyck@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=kubakici@wp.pl \
--cc=netdev@vger.kernel.org \
--cc=oss-drivers@netronome.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).