On 7/10/23 9:42 AM, Daniel Borkmann wrote:
On 7/7/23 11:27 PM, Stanislav Fomichev wrote:
On 07/07, Daniel Borkmann wrote:
[...]
+static inline struct bpf_mprog_entry *
+bpf_mprog_create(const size_t size, const off_t off)
+{
+ struct bpf_mprog_bundle *bundle;
+ void *ptr;
+
+ BUILD_BUG_ON(size < sizeof(*bundle) + off);
+ BUILD_BUG_ON(sizeof(bundle->a.fp_items[0]) > sizeof(u64));
+ BUILD_BUG_ON(ARRAY_SIZE(bundle->a.fp_items) !=
+ ARRAY_SIZE(bundle->cp_items));
+
+ ptr = kzalloc(size, GFP_KERNEL);
+ if (ptr) {
+ bundle = ptr + off;
+ atomic64_set(&bundle->revision, 1);
+ bundle->off = off;
+ bundle->a.parent = bundle;
+ bundle->b.parent = bundle;
+ return &bundle->a;
+ }
+ return NULL;
+}
+
+void bpf_mprog_free_rcu(struct rcu_head *rcu);
+
+static inline void bpf_mprog_free(struct bpf_mprog_entry *entry)
+{
+ struct bpf_mprog_bundle *bundle = entry->parent;
+
+ call_rcu(&bundle->rcu, bpf_mprog_free_rcu);
+}
Any reason we're doing allocation here? Why not do
bpf_mprog_init(struct bpf_mprog_bundle *) instead that simply initializes
the fields? Then we can move allocation/free part to the caller (tcx) along
with rcu_head.
Feels like it would be a bit more conventional/readable? bpf_mprog_free{,_rcu}
will also become tcx_free{,_rcu}..
I guess current approach works, but it took me awhile to figure it out..
(maybe it's just me)
I found this approach quite useful for tcx case since we only fetch the
bpf_mprog_entry for tcx_link_prog_attach et al, but I can take a look to
see if this looks better and if it does I'll include it.
Ok, I moved this into tcx and only left bpf_mprog_bundle_init() in mprog.
Looks better indeed, thanks Stan!
