From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 06D301F947 for ; Mon, 18 Sep 2023 18:48:06 +0000 (UTC) Received: from out-227.mta1.migadu.com (out-227.mta1.migadu.com [95.215.58.227]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 06014FC for ; Mon, 18 Sep 2023 11:48:03 -0700 (PDT) Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1695062882; 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=dpARqLkUWoTDT8Y57mg4JZyuZNPJucMNE9Iv1pxRBLA=; b=K1yZoJYJD21OZFXw1py5O8aP20jdW+gTZPGwej4S5E12oCJw0Y2Sb+DJK3B2Nn5xdthUzm 04eZQfJhJ1Tw4JCgy+fAvrNpBjJ9sIHMXfq9uQtPI5rQU+iCZY7avX0sZthGHR+60ilZTm aUVZYdpUUDLX/UH2qaIMS/NY0Fw4wWo= Date: Mon, 18 Sep 2023 11:47:55 -0700 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [RFC bpf-next v2 2/9] bpf: add register and unregister functions for struct_ops. Content-Language: en-US To: Kui-Feng Lee Cc: kuifeng@meta.com, bpf@vger.kernel.org, ast@kernel.org, song@kernel.org, kernel-team@meta.com, andrii@kernel.org, thinker.li@gmail.com References: <20230913061449.1918219-1-thinker.li@gmail.com> <20230913061449.1918219-3-thinker.li@gmail.com> <414e9f49-ad34-5282-6c05-882876440f34@linux.dev> <912b0d41-5959-74ff-a1a9-6277bf62aac2@gmail.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Martin KaFai Lau In-Reply-To: <912b0d41-5959-74ff-a1a9-6277bf62aac2@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net On 9/15/23 6:14 PM, Kui-Feng Lee wrote: > > > On 9/15/23 17:05, Martin KaFai Lau wrote: >> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: >>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >>> +{ >>> +    struct bpf_struct_ops *st_ops = mod->st_ops; >>> +    struct bpf_verifier_log *log; >>> +    struct btf *btf; >>> +    int err; >>> + >>> +    if (mod->st_ops == NULL || >>> +        mod->owner == NULL) >>> +        return -EINVAL; >>> + >>> +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >>> +    if (!log) { >>> +        err = -ENOMEM; >>> +        goto errout; >>> +    } >>> + >>> +    log->level = BPF_LOG_KERNEL; >>> + >>> +    btf = btf_get_module_btf(mod->owner); >> >> Where is btf_put called? >> >> It is not stored anywhere in patch 2, so a bit confusing. I quickly looked at >> the following patches but also don't see the bpf_put. > > It is my fault to use it without calling btf_put(). > I miss-understood the API, thought it doesn't increase refcount by > mistake. > >> >>> +    if (!btf) { >>> +        err = -EINVAL; >>> +        goto errout; >>> +    } >>> + >>> +    bpf_struct_ops_init_one(st_ops, btf, log); >>> +    err = add_struct_ops(st_ops); >>> + >>> +errout: >>> +    kfree(log); >>> + >>> +    return err; >>> +} >>> +EXPORT_SYMBOL(register_bpf_struct_ops); >>> + >>> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >> >> It is not clear to me why the subsystem needs to explicitly call >> unregister_bpf_struct_ops(). Can it be done similar to the module kfunc >> support (the kfunc_set_tab goes away with the btf)? > > It could be. However, registering to module notifier > (register_module_notifier()) is more straight forward if we go > this way. What do you think? Right, but not sure if struct_ops needs to create yet another notifier considering there is already a btf_module_notify(). It is why the earlier question on btf_put because I was trying to understand if the struct_ops can go away together during btf_free. More on this next. > >> >> Related to this, does it need to maintain a global struct_ops array for all >> kernel module? Can the struct_ops be maintained under its corresponding module >> btf itself? > > What is the purpose? > We have a global struct_ops array already, although it is not > per-module. For now, the number of struct_ops is pretty small. > We have only one so far, and it is unlikely to grow fast in > near future. It is probably a bit overkill to have > per-module ones if this is what you mean. The array size is not the concern. The global struct_ops array was created before btf supporting kernel module. Since then, btf module and kfunc module support were added. To maintain this global struct_ops array, it needs to register its own module notifier, maintains its own mutex_lock (in patch 5), and also the modified bpf_struct_ops_find*() is searching something under a specific btf module. afaict, the current btf kfunc support has the infrastructure to do all these (for example, the global LIST_HEAD(btf_modules), btf_module_mutex, btf_module_notify()...etc). Why struct_ops needs to be special and reinvent something which looks very similar to btf kfunc? Did I missing something that struct_ops needs special handling? > >> >>> +{ >>> +    struct bpf_struct_ops *st_ops = mod->st_ops; >>> +    int err; >>> + >>> +    err = remove_struct_ops(st_ops); >>> +    if (!err && st_ops->uninit) >>> +        err = st_ops->uninit(); >>> + >>> +    return err; >>> +} >>> +EXPORT_SYMBOL(unregister_bpf_struct_ops); >> >>