From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CA94C11F68 for ; Fri, 2 Jul 2021 11:20:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 26224613F7 for ; Fri, 2 Jul 2021 11:20:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231634AbhGBLXU (ORCPT ); Fri, 2 Jul 2021 07:23:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55940 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231283AbhGBLXT (ORCPT ); Fri, 2 Jul 2021 07:23:19 -0400 Received: from mail-pl1-x642.google.com (mail-pl1-x642.google.com [IPv6:2607:f8b0:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7E6A1C061762; Fri, 2 Jul 2021 04:20:47 -0700 (PDT) Received: by mail-pl1-x642.google.com with SMTP id b5so5412022plg.2; Fri, 02 Jul 2021 04:20:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=3ZOlt2GxQwZhVrq65aQ7+Tiz3iFtgGINny3AWdXdVZ4=; b=TeyXekGZwOYVAzLUn10Agi4eLQwt/E/XLvSfBCc/XOxxkB8k6+aYl25DnTolAOfvyK 5ypK6tdGT4sWNfc0HpiQezrXHlPpau8CAZglmQ329p869ENx27d0ikEtvYBfErt0fq3y KVbQD/TCwTAbKwXU6dx+JGvVKF7qhnqaJlPrxvCeeuWs4LENVI9RQuH9g58QucIix7n8 2o/WFtHxQHxO/ULGbW1xo3uLWqluJtsp5zsRFkgR9IxFp+Bx1ypXu0u/1zx+BgpWf3/Y 97VNWTJ4KFxFmxLXHK1MDAUiRNxHLF4RhBBqvIJBh3JPhRNZxzBwzwKXsfY0sREbu0mY O8gg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=3ZOlt2GxQwZhVrq65aQ7+Tiz3iFtgGINny3AWdXdVZ4=; b=kB9fBMuIIsVqXzhIzB3MPto8KK9eT6QgrfqANPGKWWf724rlUkYylXrIf/vcichuUn maHNsSufxpxpelN6QuyoIXbEanhiglqKITQCJ/XaFuYKBWxsSmAewSdmg1jb1i4K45DQ vbv6o9zNQYZmRB7VMbmHoSYWQnSNM/eOvqUAjK6OuRgoIo18zPlXs+XIAH81YNy7oZNs wgob0TN3nQCLMmxyBeiD1A7WLfSaPj7YrW2VEDTDDFjC6KaXRXFhVrQqtvtZiajJwU5V ZD+J2Z8DGtisKuR6rFup5z+0H5MumWuQfBAPq/goJAHSVj/JzptYOSwZ7t9eYie+N+m+ PNXQ== X-Gm-Message-State: AOAM530/Ua2U6vr0YBLEvLGacf8DoRFf1xeE2HxI84S177d++Wtw9Rri UjvoLmu8I361R7xUKUUO8ik= X-Google-Smtp-Source: ABdhPJym6PAyLRAwi0D4o8bpZ9mLL1Z0fryO7nvg5SnzNwodIjo/oat9uPcinOJ5Eoe5F8ny4MgXrg== X-Received: by 2002:a17:90b:4ac8:: with SMTP id mh8mr4548717pjb.84.1625224847029; Fri, 02 Jul 2021 04:20:47 -0700 (PDT) Received: from localhost ([2409:4063:4d83:c0b5:70cd:e919:ab0c:33ce]) by smtp.gmail.com with ESMTPSA id j20sm3123489pfc.85.2021.07.02.04.20.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Jul 2021 04:20:46 -0700 (PDT) Date: Fri, 2 Jul 2021 16:48:57 +0530 From: Kumar Kartikeya Dwivedi To: Jesper Dangaard Brouer Cc: netdev@vger.kernel.org, Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , "David S. Miller" , Jakub Kicinski , John Fastabend , Martin KaFai Lau , bpf@vger.kernel.org, Eric Leblond Subject: Re: [PATCH net-next v5 3/5] bpf: cpumap: implement generic cpumap Message-ID: <20210702111857.jfmeuxnkwg6v7krk@apollo> References: <20210701002759.381983-1-memxor@gmail.com> <20210701002759.381983-4-memxor@gmail.com> <3b80be91-41f2-5c5d-4e93-e6290812f756@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <3b80be91-41f2-5c5d-4e93-e6290812f756@redhat.com> Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Thu, Jul 01, 2021 at 02:46:05PM IST, Jesper Dangaard Brouer wrote: > (Cc. Eric Leblond as he needed this for Suricata.) > > On 01/07/2021 02.27, Kumar Kartikeya Dwivedi wrote: > > This change implements CPUMAP redirect support for generic XDP programs. > > The idea is to reuse the cpu map entry's queue that is used to push > > native xdp frames for redirecting skb to a different CPU. This will > > match native XDP behavior (in that RPS is invoked again for packet > > reinjected into networking stack). > > > > To be able to determine whether the incoming skb is from the driver or > > cpumap, we reuse skb->redirected bit that skips generic XDP processing > > when it is set. To always make use of this, CONFIG_NET_REDIRECT guard on > > it has been lifted and it is always available. > > > > From the redirect side, we add the skb to ptr_ring with its lowest bit > > set to 1. This should be safe as skb is not 1-byte aligned. This allows > > kthread to discern between xdp_frames and sk_buff. On consumption of the > > ptr_ring item, the lowest bit is unset. > > > > In the end, the skb is simply added to the list that kthread is anyway > > going to maintain for xdp_frames converted to skb, and then received > > again by using netif_receive_skb_list. > > > > Bulking optimization for generic cpumap is left as an exercise for a > > future patch for now. > Fine by me, I hope bulking is added later, as I think with bulking this will > be a faster alternative than RPS. > > > > Since cpumap entry progs are now supported, also remove check in > > generic_xdp_install for the cpumap. > > > > Reviewed-by: Toke Høiland-Jørgensen > > Signed-off-by: Kumar Kartikeya Dwivedi > > --- > > include/linux/bpf.h | 9 +++- > > include/linux/skbuff.h | 10 +--- > > kernel/bpf/cpumap.c | 115 +++++++++++++++++++++++++++++++++++------ > > net/core/dev.c | 3 +- > > net/core/filter.c | 6 ++- > > 5 files changed, 115 insertions(+), 28 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index f309fc1509f2..095aaa104c56 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1513,7 +1513,8 @@ bool dev_map_can_have_prog(struct bpf_map *map); > > void __cpu_map_flush(void); > > int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp, > > struct net_device *dev_rx); > > -bool cpu_map_prog_allowed(struct bpf_map *map); > > +int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu, > > + struct sk_buff *skb); > > /* Return map's numa specified by userspace */ > > static inline int bpf_map_attr_numa_node(const union bpf_attr *attr) > > @@ -1710,6 +1711,12 @@ static inline int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, > > return 0; > > } > > +static inline int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu, > > + struct sk_buff *skb) > > +{ > > + return -EOPNOTSUPP; > > +} > > + > > static inline bool cpu_map_prog_allowed(struct bpf_map *map) > > { > > return false; > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index b2db9cd9a73f..f19190820e63 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -863,8 +863,8 @@ struct sk_buff { > > __u8 tc_skip_classify:1; > > __u8 tc_at_ingress:1; > > #endif > > -#ifdef CONFIG_NET_REDIRECT > > __u8 redirected:1; > > +#ifdef CONFIG_NET_REDIRECT > > __u8 from_ingress:1; > > #endif > > #ifdef CONFIG_TLS_DEVICE > > @@ -4664,17 +4664,13 @@ static inline __wsum lco_csum(struct sk_buff *skb) > > static inline bool skb_is_redirected(const struct sk_buff *skb) > > { > > -#ifdef CONFIG_NET_REDIRECT > > return skb->redirected; > > -#else > > - return false; > > -#endif > > } > > static inline void skb_set_redirected(struct sk_buff *skb, bool from_ingress) > > { > > -#ifdef CONFIG_NET_REDIRECT > > skb->redirected = 1; > > +#ifdef CONFIG_NET_REDIRECT > > skb->from_ingress = from_ingress; > > if (skb->from_ingress) > > skb->tstamp = 0; > > @@ -4683,9 +4679,7 @@ static inline void skb_set_redirected(struct sk_buff *skb, bool from_ingress) > > static inline void skb_reset_redirect(struct sk_buff *skb) > > { > > -#ifdef CONFIG_NET_REDIRECT > > skb->redirected = 0; > > -#endif > > } > > static inline bool skb_csum_is_sctp(struct sk_buff *skb) > > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c > > index a1a0c4e791c6..274353e2cd70 100644 > > --- a/kernel/bpf/cpumap.c > > +++ b/kernel/bpf/cpumap.c > > @@ -16,6 +16,7 @@ > > * netstack, and assigning dedicated CPUs for this stage. This > > * basically allows for 10G wirespeed pre-filtering via bpf. > > */ > > +#include > > #include > > #include > > #include > > @@ -168,6 +169,49 @@ static void put_cpu_map_entry(struct bpf_cpu_map_entry *rcpu) > > } > > } > > +static void cpu_map_bpf_prog_run_skb(struct bpf_cpu_map_entry *rcpu, > > + struct list_head *listp, > > + struct xdp_cpumap_stats *stats) > > +{ > > + struct sk_buff *skb, *tmp; > > + struct xdp_buff xdp; > > + u32 act; > > + int err; > > + > > + if (!rcpu->prog) > > + return; > > Move this one level out. Why explained below. > > > > + > > + list_for_each_entry_safe(skb, tmp, listp, list) { > > + act = bpf_prog_run_generic_xdp(skb, &xdp, rcpu->prog); > > + switch (act) { > > + case XDP_PASS: > > + break; > > + case XDP_REDIRECT: > > + skb_list_del_init(skb); > > + err = xdp_do_generic_redirect(skb->dev, skb, &xdp, > > + rcpu->prog); > > + if (unlikely(err)) { > > + kfree_skb(skb); > > + stats->drop++; > > + } else { > > + stats->redirect++; > > + } > > + return; > > + default: > > + bpf_warn_invalid_xdp_action(act); > > + fallthrough; > > + case XDP_ABORTED: > > + trace_xdp_exception(skb->dev, rcpu->prog, act); > > + fallthrough; > > + case XDP_DROP: > > + skb_list_del_init(skb); > > + kfree_skb(skb); > > + stats->drop++; > > + return; > > + } > > + } > > +} > > + > > static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu, > > void **frames, int n, > > struct xdp_cpumap_stats *stats) > > @@ -179,8 +223,6 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu, > > if (!rcpu->prog) > > return n; > > - rcu_read_lock_bh(); > > - > > Notice the return before doing rcu_read_lock_bh(). > > Here we try to avoid the extra call to do_softirq() when calling > rcu_read_unlock_bh. > > When RX-napi and cpumap share/run-on the same CPU, activating do_softirq() > two time in the kthread_cpumap cause RX-napi to get more CPU time to enqueue > more packets into cpumap.  Thus, cpumap can easier get overloaded. > > > > xdp_set_return_frame_no_direct(); > > xdp.rxq = &rxq; > > @@ -227,17 +269,34 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu, > > } > > } > > + xdp_clear_return_frame_no_direct(); > > + > > + return nframes; > > +} > > + > > +#define CPUMAP_BATCH 8 > > + > > +static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames, > > + int xdp_n, struct xdp_cpumap_stats *stats, > > + struct list_head *list) > > +{ > > + int nframes; > > + > > + rcu_read_lock_bh(); > > + > > + nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats); > > + > > if (stats->redirect) > > - xdp_do_flush_map(); > > + xdp_do_flush(); > > - xdp_clear_return_frame_no_direct(); > > + if (unlikely(!list_empty(list))) > > + cpu_map_bpf_prog_run_skb(rcpu, list, stats); > > - rcu_read_unlock_bh(); /* resched point, may call do_softirq() */ > > + rcu_read_unlock_bh(); > > I would like to keep this comment, to help people troubleshooting/understand > why RX-napi to get more CPU time than kthread. > > > > return nframes; > > } > > -#define CPUMAP_BATCH 8 > > static int cpu_map_kthread_run(void *data) > > { > > @@ -254,9 +313,9 @@ static int cpu_map_kthread_run(void *data) > > struct xdp_cpumap_stats stats = {}; /* zero stats */ > > unsigned int kmem_alloc_drops = 0, sched = 0; > > gfp_t gfp = __GFP_ZERO | GFP_ATOMIC; > > + int i, n, m, nframes, xdp_n; > > void *frames[CPUMAP_BATCH]; > > void *skbs[CPUMAP_BATCH]; > > - int i, n, m, nframes; > > LIST_HEAD(list); > > /* Release CPU reschedule checks */ > > @@ -280,9 +339,20 @@ static int cpu_map_kthread_run(void *data) > > */ > > n = __ptr_ring_consume_batched(rcpu->queue, frames, > > CPUMAP_BATCH); > > - for (i = 0; i < n; i++) { > > + for (i = 0, xdp_n = 0; i < n; i++) { > > void *f = frames[i]; > > - struct page *page = virt_to_page(f); > > + struct page *page; > > + > > + if (unlikely(__ptr_test_bit(0, &f))) { > > + struct sk_buff *skb = f; > > + > > + __ptr_clear_bit(0, &skb); > > + list_add_tail(&skb->list, &list); > > + continue; > > + } > > + > > + frames[xdp_n++] = f; > > + page = virt_to_page(f); > > /* Bring struct page memory area to curr CPU. Read by > > * build_skb_around via page_is_pfmemalloc(), and when > > @@ -292,7 +362,7 @@ static int cpu_map_kthread_run(void *data) > > } > > /* Support running another XDP prog on this CPU */ > > - nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, n, &stats); > > + nframes = cpu_map_bpf_prog_run(rcpu, frames, xdp_n, &stats, &list); > > if (nframes) { > > m = kmem_cache_alloc_bulk(skbuff_head_cache, gfp, nframes, skbs); > > if (unlikely(m == 0)) { > > @@ -330,12 +400,6 @@ static int cpu_map_kthread_run(void *data) > > return 0; > > } > > -bool cpu_map_prog_allowed(struct bpf_map *map) > > -{ > > - return map->map_type == BPF_MAP_TYPE_CPUMAP && > > - map->value_size != offsetofend(struct bpf_cpumap_val, qsize); > > -} > > - > > static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, int fd) > > { > > struct bpf_prog *prog; > > @@ -696,6 +760,25 @@ int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp, > > return 0; > > } > > +int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu, > > + struct sk_buff *skb) > > +{ > > + int ret; > > + > > + __skb_pull(skb, skb->mac_len); > > + skb_set_redirected(skb, false); > > + __ptr_set_bit(0, &skb); > > + > > + ret = ptr_ring_produce(rcpu->queue, skb); > > + if (ret < 0) > > + goto trace; > > + > > + wake_up_process(rcpu->kthread); > > +trace: > > + trace_xdp_cpumap_enqueue(rcpu->map_id, !ret, !!ret, rcpu->cpu); > > + return ret; > > +} > > + > > void __cpu_map_flush(void) > > { > > struct list_head *flush_list = this_cpu_ptr(&cpu_map_flush_list); > > diff --git a/net/core/dev.c b/net/core/dev.c > > index ad5ab33cbd39..8521936414f2 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -5665,8 +5665,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp) > > * have a bpf_prog installed on an entry > > */ > > for (i = 0; i < new->aux->used_map_cnt; i++) { > > - if (dev_map_can_have_prog(new->aux->used_maps[i]) || > > - cpu_map_prog_allowed(new->aux->used_maps[i])) { > > + if (dev_map_can_have_prog(new->aux->used_maps[i])) { > > mutex_unlock(&new->aux->used_maps_mutex); > > return -EINVAL; > > } > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 0b13d8157a8f..4a21fde3028f 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -4038,8 +4038,12 @@ static int xdp_do_generic_redirect_map(struct net_device *dev, > > goto err; > > consume_skb(skb); > > break; > > + case BPF_MAP_TYPE_CPUMAP: > > + err = cpu_map_generic_redirect(fwd, skb); > > + if (unlikely(err)) > > + goto err; > > + break; > > default: > > - /* TODO: Handle BPF_MAP_TYPE_CPUMAP */ > > err = -EBADRQC; > > goto err; > > } > > > I like the rest :-) > > Thanks for working on this! > > --Jesper > Hopefully addressed both points in the respin, thanks! -- Kartikeya