On 3/3/24 9:20 AM, Jamal Hadi Salim wrote:
+#define P4TC_MAX_PARAM_DATA_SIZE 124
+
+struct p4tc_table_entry_act_bpf {
+ u32 act_id;
+ u32 hit:1,
+ is_default_miss_act:1,
+ is_default_hit_act:1;
+ u8 params[P4TC_MAX_PARAM_DATA_SIZE];
+} __packed;
+
+struct p4tc_table_entry_act_bpf_kern {
+ struct rcu_head rcu;
+ struct p4tc_table_entry_act_bpf act_bpf;
+};
+
struct tcf_p4act {
struct tc_action common;
/* Params IDR reference passed during runtime */
struct tcf_p4act_params __rcu *params;
+ struct p4tc_table_entry_act_bpf_kern __rcu *act_bpf;
u32 p_id;
u32 act_id;
struct list_head node;
@@ -24,4 +40,39 @@ struct tcf_p4act {
#define to_p4act(a) ((struct tcf_p4act *)a)
+static inline struct p4tc_table_entry_act_bpf *
+p4tc_table_entry_act_bpf(struct tc_action *action)
+{
+ struct p4tc_table_entry_act_bpf_kern *act_bpf;
+ struct tcf_p4act *p4act = to_p4act(action);
+
+ act_bpf = rcu_dereference(p4act->act_bpf);
+
+ return &act_bpf->act_bpf;
+}
+
+static inline int
+p4tc_table_entry_act_bpf_change_flags(struct tc_action *action, u32 hit,
+ u32 dflt_miss, u32 dflt_hit)
+{
+ struct p4tc_table_entry_act_bpf_kern *act_bpf, *act_bpf_old;
+ struct tcf_p4act *p4act = to_p4act(action);
+
+ act_bpf = kzalloc(sizeof(*act_bpf), GFP_KERNEL);
[ ... ]
+static int
+__bpf_p4tc_entry_create(struct net *net,
+ struct p4tc_table_entry_create_bpf_params *params,
+ void *key, const u32 key__sz,
+ struct p4tc_table_entry_act_bpf *act_bpf)
+{
+ struct p4tc_table_entry_key *entry_key = key;
+ struct p4tc_pipeline *pipeline;
+ struct p4tc_table *table;
+
+ if (!params || !key)
+ return -EINVAL;
+ if (key__sz != P4TC_ENTRY_KEY_SZ_BYTES(entry_key->keysz))
+ return -EINVAL;
+
+ pipeline = p4tc_pipeline_find_byid(net, params->pipeid);
+ if (!pipeline)
+ return -ENOENT;
+
+ table = p4tc_tbl_cache_lookup(net, params->pipeid, params->tblid);
+ if (!table)
+ return -ENOENT;
+
+ if (entry_key->keysz != table->tbl_keysz)
+ return -EINVAL;
+
+ return p4tc_table_entry_create_bpf(pipeline, table, entry_key, act_bpf,
+ params->profile_id);
My understanding is this kfunc will allocate a "struct
p4tc_table_entry_act_bpf_kern" object. If the bpf_p4tc_entry_delete() kfunc is
never called and the bpf prog is unloaded, how the act_bpf object will be
cleaned up?
The TC code takes care of this. Unloading the bpf prog does not affect
the deletion, it is the TC control side that will take care of it. If
we delete the pipeline otoh then not just this entry but all entries
will be flushed.
It looks like the "struct p4tc_table_entry_act_bpf_kern" object is allocated by
the bpf prog through kfunc and will only be useful for the bpf prog but not
other parts of the kernel. However, if the bpf prog is unloaded, these bpf
specific objects will be left over in the kernel until the tc pipeline (where
the act_bpf_kern object resided) is gone.
It is the expectation on bpf prog (not only tc/xdp bpf prog) about resources
clean up that these bpf objects will be gone after unloading the bpf prog and
unpinning its bpf map.
The table (residing on the TC side) could be shared by multiple bpf
programs. Entries are allocated on the TC side of the fence.
IOW, the memory is not owned by the bpf prog but rather by pipeline.
The struct p4tc_table_entry_act_(bpf_kern) object is allocated by
bpf_p4tc_entry_create() kfunc and only bpf prog can use it, no?
afaict, this is bpf objects.
We do have a "whodunnit" field, i.e we keep track of which entity
added an entry and we are capable of deleting all entries when we
detect a bpf program being deleted (this would be via deleting the tc
filter). But my thinking is we should make that a policy decision as
opposed to something which is default.
afaik, this policy decision or cleanup upon tc filter delete has not been done
yet. I will leave it to you to figure out how to track what was allocated by a
particular bpf prog on the TC side. It is not immediately clear to me and I
probably won't have a good idea either.
Just to be clear that it is almost certain to be unacceptable to extend and make
changes on the bpf side in the future to handle specific resource
cleanup/tracking/sharing of the bpf objects allocated by these kfuncs. This
problem has already been solved and works for different bpf program types,
tc/cgroup/tracing...etc. Adding a refcnted bpf prog pointer alongside the
act_bpf_kern object will be a non-starter.
I think multiple people have already commented that these kfuncs
(create/update/delete...) resemble the existing bpf map. If these kfuncs are
replaced with the bpf map ops, this bpf resource management has already been
handled and will be consistent with other bpf program types.
I expect the act_bpf_kern object probably will grow in size over time also.
Considering this new p4 pipeline and table is residing on the TC side, I will
leave it up to others to decide if it is acceptable to have some unused bpf
objects left attached there.
