From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laatz, Kevin Date: Fri, 26 Jul 2019 10:47:11 +0100 Subject: [Intel-wired-lan] [PATCH bpf-next v3 03/11] xsk: add support to allow unaligned chunk placement In-Reply-To: References: <20190716030637.5634-1-kevin.laatz@intel.com> <20190724051043.14348-1-kevin.laatz@intel.com> <20190724051043.14348-4-kevin.laatz@intel.com> Message-ID: <096504af-35a9-7604-7c06-e500224256ea@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On 25/07/2019 16:39, Jonathan Lemon wrote: > On 23 Jul 2019, at 22:10, Kevin Laatz wrote: > >> Currently, addresses are chunk size aligned. This means, we are very >> restricted in terms of where we can place chunk within the umem. For >> example, if we have a chunk size of 2k, then our chunks can only be >> placed >> at 0,2k,4k,6k,8k... and so on (ie. every 2k starting from 0). >> >> This patch introduces the ability to use unaligned chunks. With these >> changes, we are no longer bound to having to place chunks at a 2k (or >> whatever your chunk size is) interval. Since we are no longer dealing >> with >> aligned chunks, they can now cross page boundaries. Checks for page >> contiguity have been added in order to keep track of which pages are >> followed by a physically contiguous page. >> >> Signed-off-by: Kevin Laatz >> Signed-off-by: Ciara Loftus >> Signed-off-by: Bruce Richardson >> >> --- >> v2: >> ? - Add checks for the flags coming from userspace >> ? - Fix how we get chunk_size in xsk_diag.c >> ? - Add defines for masking the new descriptor format >> ? - Modified the rx functions to use new descriptor format >> ? - Modified the tx functions to use new descriptor format >> >> v3: >> ? - Add helper function to do address/offset masking/addition >> --- >> ?include/net/xdp_sock.h????? | 17 ++++++++ >> ?include/uapi/linux/if_xdp.h |? 9 ++++ >> ?net/xdp/xdp_umem.c????????? | 18 +++++--- >> ?net/xdp/xsk.c?????????????? | 86 ++++++++++++++++++++++++++++++------- >> ?net/xdp/xsk_diag.c????????? |? 2 +- >> ?net/xdp/xsk_queue.h???????? | 68 +++++++++++++++++++++++++---- >> ?6 files changed, 170 insertions(+), 30 deletions(-) >> >> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h >> index 69796d264f06..738996c0f995 100644 >> --- a/include/net/xdp_sock.h >> +++ b/include/net/xdp_sock.h >> @@ -19,6 +19,7 @@ struct xsk_queue; >> ?struct xdp_umem_page { >> ???? void *addr; >> ???? dma_addr_t dma; >> +??? bool next_pg_contig; >> ?}; >> >> ?struct xdp_umem_fq_reuse { >> @@ -48,6 +49,7 @@ struct xdp_umem { >> ???? bool zc; >> ???? spinlock_t xsk_list_lock; >> ???? struct list_head xsk_list; >> +??? u32 flags; >> ?}; >> >> ?struct xdp_sock { >> @@ -144,6 +146,15 @@ static inline void xsk_umem_fq_reuse(struct >> xdp_umem *umem, u64 addr) >> >> ???? rq->handles[rq->length++] = addr; >> ?} >> + >> +static inline u64 xsk_umem_handle_offset(struct xdp_umem *umem, u64 >> handle, >> +???????????????????? u64 offset) >> +{ >> +??? if (umem->flags & XDP_UMEM_UNALIGNED_CHUNKS) >> +??????? return handle |= (offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT); >> +??? else >> +??????? return handle += offset; >> +} > > This should be something like 'xsk_umem_adjust_offset()', and use "+=" > for both cases. I'll try to come up with a better name for this, I see how "handle_offset" could be misleading :) In terms of the "+=", we can't do that for both. We need to use | for the unaligned case since we store the offset in the upper 16-bits of the address field so that we leave the lower 48-bits (the original base address) untouched. <...> >> >> ???? if (!PAGE_ALIGNED(addr)) { >> @@ -331,9 +335,11 @@ static int xdp_umem_reg(struct xdp_umem *umem, >> struct xdp_umem_reg *mr) >> ???? if (chunks == 0) >> ???????? return -EINVAL; >> >> -??? chunks_per_page = PAGE_SIZE / chunk_size; >> -??? if (chunks < chunks_per_page || chunks % chunks_per_page) >> -??????? return -EINVAL; >> +??? if (!unaligned_chunks) { >> +??????? chunks_per_page = PAGE_SIZE / chunk_size; >> +??????? if (chunks < chunks_per_page || chunks % chunks_per_page) >> +??????????? return -EINVAL; >> +??? } >> >> ???? headroom = ALIGN(headroom, 64); >> >> @@ -342,13 +348,15 @@ static int xdp_umem_reg(struct xdp_umem *umem, >> struct xdp_umem_reg *mr) >> ???????? return -EINVAL; >> >> ???? umem->address = (unsigned long)addr; >> -??? umem->chunk_mask = ~((u64)chunk_size - 1); >> +??? umem->chunk_mask = unaligned_chunks ? XSK_UNALIGNED_BUF_ADDR_MASK >> +??????????????????????? : ~((u64)chunk_size - 1); > > The handle needs to be cleaned (reset to base address) when removed > from the fill queue or recycle stack.? This will not provide the correct > semantics for unaligned mode. When we use aligned mode, the chunk mask is ~0x07FF which will remove the low 11-bits in order to get us back to the original base address. In unaligned mode, the chunk mask is ~0xFFFF00....00 which will remove the upper 16-bits. This will remove the offset from the address field and, since we have not directly modified the low 48-bits (original base address), leave us with the original base address. Cleaning the handle will therefore work as it does now, using the appropriate mask based on which mode we are using. <...> >> >> ???????? if (xskq_produce_addr_lazy(umem->cq, desc->addr)) >> @@ -243,7 +269,7 @@ static int xsk_generic_xmit(struct sock *sk, >> struct msghdr *m, >> ???? if (xs->queue_id >= xs->dev->real_num_tx_queues) >> ???????? goto out; >> >> -??? while (xskq_peek_desc(xs->tx, &desc)) { >> +??? while (xskq_peek_desc(xs->tx, &desc, xs->umem)) { >> ???????? char *buffer; >> ???????? u64 addr; >> ???????? u32 len; >> @@ -262,6 +288,10 @@ static int xsk_generic_xmit(struct sock *sk, >> struct msghdr *m, >> >> ???????? skb_put(skb, len); >> ???????? addr = desc.addr; >> +??????? if (xs->umem->flags & XDP_UMEM_UNALIGNED_CHUNKS) >> +??????????? addr = (addr & XSK_UNALIGNED_BUF_ADDR_MASK) | >> +??????????????? (addr >> XSK_UNALIGNED_BUF_OFFSET_SHIFT); > > This doesn't look right to me.? Shouldn't it be "(addr & mask) + (addr > >> shift)"? > I'd also prefer to see this type of logic in an inline/macro Will fix in the v4, thanks! I'll look to move this to an inline function as well. <...> -------------- next part -------------- An HTML attachment was scrubbed... URL: