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=-17.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 CFA60C11F64 for ; Thu, 1 Jul 2021 09:16:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AD5CD61490 for ; Thu, 1 Jul 2021 09:16:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235609AbhGAJSl (ORCPT ); Thu, 1 Jul 2021 05:18:41 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:45707 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235497AbhGAJSl (ORCPT ); Thu, 1 Jul 2021 05:18:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1625130970; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RwAN9Fk3DO6mr4H8sxlc4mxUEwHN0v2CqyZHwyiaUkY=; b=UTGQXrX/lRWkmcuRvQNnsn7aLAwm4DTTV6/W0Rlv6YUSpBHocdiEg/YvhGcsOOn/sx6erL VO0MC1ENzZ5rlzehLTAMtOyqLNLqDoFcci4krloHuxFqs+XOd500xSSE2kKPbjO6Z1jMnD w9hHWPkiE50n93O/h61n5wchVTb4UYk= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-432-rdTEZNNuN5OVGiYTCDFmNA-1; Thu, 01 Jul 2021 05:16:09 -0400 X-MC-Unique: rdTEZNNuN5OVGiYTCDFmNA-1 Received: by mail-ed1-f71.google.com with SMTP id p19-20020aa7c4d30000b0290394bdda6d9cso2701503edr.21 for ; Thu, 01 Jul 2021 02:16:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=RwAN9Fk3DO6mr4H8sxlc4mxUEwHN0v2CqyZHwyiaUkY=; b=FLRuh4bhjqqpBPuU8lWUHiOY+WtMOP4rrX42026NBc2xAznyJeqgYRr/y51UocT1MI /W8rnAkPBQpjDZ5WDQ/ZAaZiYmICtT13ftXcRoycwglpbLUVw2SHAjvqCs+loRIToz61 L+41uFvz4cNB/xlikkwrOnBKQ1hPkzNVhsr+T2hU3OX/4i38gSoYjUb9+dy0z0J8OaPj mrozfQxkWhoPXX3I9UhoQM6o9BKaKD1tOFz9PzoHD2QWWTuQZalFnHAHviSS3W8xSje2 pZKV/NS2zYQNUp7oXd9xUfR6PAd9NTvc8XfpDU59fp0rtkjYgyCnZ8AwoHTAW9uWzsD3 Eabw== X-Gm-Message-State: AOAM530SVRrAnX8Un/e83kVkEF695ui8zTx7MqxxMtD/8RftLMfXlPXP Gcpg1FNQmATRtlOTCGPOFxoGcmgWUU4rF93CseryrcuYtBODNnWYIzJKzAuYAj0ku0h30TeUZlE gTGGiVAj4Z4JF X-Received: by 2002:a05:6402:90c:: with SMTP id g12mr48181862edz.371.1625130968217; Thu, 01 Jul 2021 02:16:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzgR32zc39DJneORe2We+YRgJH1E6If1sCkciWxK1AL51Tu6QMiCgta8EnCqYqAGtiS9nEB6A== X-Received: by 2002:a05:6402:90c:: with SMTP id g12mr48181836edz.371.1625130967948; Thu, 01 Jul 2021 02:16:07 -0700 (PDT) Received: from [192.168.42.238] (3-14-107-185.static.kviknet.dk. [185.107.14.3]) by smtp.gmail.com with ESMTPSA id b17sm4547546edd.58.2021.07.01.02.16.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 01 Jul 2021 02:16:06 -0700 (PDT) From: Jesper Dangaard Brouer X-Google-Original-From: Jesper Dangaard Brouer Subject: Re: [PATCH net-next v5 3/5] bpf: cpumap: implement generic cpumap To: Kumar Kartikeya Dwivedi , netdev@vger.kernel.org Cc: =?UTF-8?Q?Toke_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 References: <20210701002759.381983-1-memxor@gmail.com> <20210701002759.381983-4-memxor@gmail.com> Message-ID: <3b80be91-41f2-5c5d-4e93-e6290812f756@redhat.com> Date: Thu, 1 Jul 2021 11:16:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210701002759.381983-4-memxor@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org (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