From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) by mail.toke.dk (Postfix) with ESMTPS id 37DA29CD38F for ; Fri, 9 Dec 2022 00:46:02 +0100 (CET) Authentication-Results: mail.toke.dk; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=D2e2V1ii Received: by mail-pj1-x102e.google.com with SMTP id e7-20020a17090a77c700b00216928a3917so6292454pjs.4 for ; Thu, 08 Dec 2022 15:46:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Sm8OWFrvWQaS6H9xrHvPh1htLWtKmKkj+nrb35AKVrk=; b=D2e2V1iieBFHio2OsHZaVCyOeG45+/qZ+cdKD5J5/yOJB0c6+ZSxMEKPyTaPR+wPAO mbuHXoYELeSjFhP6JKrA61pthNzve46uS3LCf9ZRQDzAIA5Cn693fwk+6aTTdeChQbMp vTKC2AhdN8pKe/PqqVaAoyagMC6xqBTxKR19PmoFA52gXfIMXiMC4BN+yMhy0uDWmlU5 Qnv1dc8n9aK1oxw8wQPuSolhIdpRQnPb5DF5tLuQTwGsi2dT8T83MyFcTH0DCN7VlZz4 uCZYi5AMyK6kw+ulRH0Jw+fuIH41Dr6IlSU++BM/yiYKXFmVVwPZzEwZPW8sMAfacI6s 856Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Sm8OWFrvWQaS6H9xrHvPh1htLWtKmKkj+nrb35AKVrk=; b=Hs5U/oEFLZ8l+RHHzCDtr1Ki0gexepuhd+MnP6UhapGZyl0Ti0puGl1aDQY9pKchs0 55mO8WnMIhF9wkIiDfaZez2Xfjd4gvTf7Tzy13+oxSzzSgyq8FQwNa2hBJYni3ltBoXS WAjpW65sJxsNdDE3eRJjZK1XrPGG9/8r1+Pt3PC0bHuEPmJEN0TjbKa9a0jErEgep2nO 9OVwYSwW9KnfM53fQDixctoSWjyYig7RjR/vRQNv0ys4GOBsEDjAJCEeAcrqDtFW8EtD dTcpGsWxN7JnHuaFGC9ffbLCt8M9PdoPfccJkiYVi7oct7yYhoz4nG0Wa5pL8ee6Oear c42g== X-Gm-Message-State: ANoB5pkSgMMPOh4D5hM9PJeNaplcLgR8HSm82xJ0XkiKwYbtR6PaFdVH Hd1g7ifdCF97CZ7JQoQgbc+2jxmAmimKOrHFrds71Q== X-Google-Smtp-Source: AA0mqf71LHOqUi6G/gW4y3OPGyoVlC3ndfi32vjtlQwBVRcYyxG4dYg5Jw7MX9r9cKx78860x1sOK0N7h6XkWTZbkq0= X-Received: by 2002:a17:90a:930f:b0:218:9107:381b with SMTP id p15-20020a17090a930f00b002189107381bmr97468563pjo.75.1670543161020; Thu, 08 Dec 2022 15:46:01 -0800 (PST) MIME-Version: 1.0 References: <20221206024554.3826186-1-sdf@google.com> <20221206024554.3826186-4-sdf@google.com> <391b9abf-c53a-623c-055f-60768c716baa@linux.dev> In-Reply-To: From: Stanislav Fomichev Date: Thu, 8 Dec 2022 15:45:49 -0800 Message-ID: To: Martin KaFai Lau Content-Type: text/plain; charset="UTF-8" Message-ID-Hash: GL63G6SQYWIKWBGVL45KL3A2RRNVHBZX X-Message-ID-Hash: GL63G6SQYWIKWBGVL45KL3A2RRNVHBZX X-MailFrom: sdf@google.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org, David Ahern , Jakub Kicinski , Willem de Bruijn , Jesper Dangaard Brouer , Anatoly Burakov , Alexander Lobakin , Magnus Karlsson , Maryam Tahhan , xdp-hints@xdp-project.net, netdev@vger.kernel.org, bpf@vger.kernel.org X-Mailman-Version: 3.3.7 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, Dec 8, 2022 at 2:53 PM Martin KaFai Lau wrote: > > On 12/8/22 11:07 AM, Stanislav Fomichev wrote: > >>> @@ -102,11 +112,25 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr) > >>> if (err) > >>> goto err_maybe_put; > >>> > >>> + prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_HAS_METADATA); > >>> + > >> > >> If I read the set correctly, bpf prog can either use metadata kfunc or offload > >> but not both. It is fine to start with only supporting metadata kfunc when there > >> is no offload but will be useful to understand the reason. I assume an offloaded > >> bpf prog should still be able to call the bpf helpers like adjust_head/tail and > >> the same should go for any kfunc? > > > > Yes, I'm assuming there should be some work on the offloaded device > > drivers to support metadata kfuncs. > > Offloaded kfuncs, in general, seem hard (how do we call kernel func > > from the device-offloaded prog?); so refusing kfuncs early for the > > offloaded case seems fair for now? > > Ah, ok. I was actually thinking the HW offloaded prog can just use the software > ndo_* kfunc (like other bpf-helpers). From skimming some > bpf_prog_offload_ops:prepare implementation, I think you are right and it seems > BPF_PSEUDO_KFUNC_CALL has not been recognized yet. > > [ ... ] > > >>> @@ -226,10 +263,17 @@ static void __bpf_prog_offload_destroy(struct bpf_prog *prog) > >>> > >>> void bpf_prog_offload_destroy(struct bpf_prog *prog) > >>> { > >>> + struct net_device *netdev = NULL; > >>> + > >>> down_write(&bpf_devs_lock); > >>> - if (prog->aux->offload) > >>> + if (prog->aux->offload) { > >>> + netdev = prog->aux->offload->netdev; > >>> __bpf_prog_offload_destroy(prog); > >>> + } > >>> up_write(&bpf_devs_lock); > >>> + > >>> + if (netdev) > >> > >> May be I have missed a refcnt or lock somewhere. Is it possible that netdev may > >> have been freed? > > > > Yeah, with the offload framework, there are no refcnts. We put an > > "offloaded" device into a separate hashtable (protected by > > rtnl/semaphore). > > maybe_remove_bound_netdev will re-grab the locks (due to ordering: > > rtnl->bpf_devs_lock) and remove the device from the hashtable if it's > > still there. > > At least this is how, I think, it should work; LMK if something is > > still fishy here... > > > > Or is the concern here that somebody might allocate new netdev reusing > > the same address? I think I have enough checks in > > maybe_remove_bound_netdev to guard against that. Or, at least, to make > > it safe :-) > > Race is ok because ondev needs to be removed anyway when '!ondev->offdev && > list_empty(&ondev->progs)'? hmmm... tricky, please add a comment. :) > > Why it cannot be done together in the bpf_devs_lock above? The above cannot > take an extra rtnl_lock before bpf_devs_lock? Hm, let's take an extra rtln to avoid this complexity, agree. I guess I was trying to avoid taking it, but this path is still 'dev_bound == true' protected, so shouldn't affect the rest of the progs.