| Message ID | 1553623539-15474-2-git-send-email-jiong.wang@netronome.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: eliminate zero extensions for sub-register writes | expand |
On Tue, Mar 26, 2019 at 06:05:24PM +0000, Jiong Wang wrote: > "enum bpf_reg_liveness" is actually used as bit instead of integer. For > example: > > if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN | REG_LIVE_DONE)) > > Using enum to represent bit is error prone, better to use explicit bit > macros. > > Signed-off-by: Jiong Wang <jiong.wang@netronome.com> > --- > include/linux/bpf_verifier.h | 16 +++++++++------- > kernel/bpf/verifier.c | 5 ++--- > 2 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 7d8228d..f03c86a 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -34,12 +34,14 @@ > * but of the link between it and its parent. See mark_reg_read() and > * mark_stack_slot_read() in kernel/bpf/verifier.c. > */ > -enum bpf_reg_liveness { > - REG_LIVE_NONE = 0, /* reg hasn't been read or written this branch */ > - REG_LIVE_READ, /* reg was read, so we're sensitive to initial value */ > - REG_LIVE_WRITTEN, /* reg was written first, screening off later reads */ > - REG_LIVE_DONE = 4, /* liveness won't be updating this register anymore */ > -}; yes. it is enum that is used as a bitfield, but I prefer to keep it as enum because clang -Wassign-enum can do at least some type checking. I also find it easier to review the code when it has 'enum bpf_reg_liveness' instead of 'u8'
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 7d8228d..f03c86a 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -34,12 +34,14 @@ * but of the link between it and its parent. See mark_reg_read() and * mark_stack_slot_read() in kernel/bpf/verifier.c. */ -enum bpf_reg_liveness { - REG_LIVE_NONE = 0, /* reg hasn't been read or written this branch */ - REG_LIVE_READ, /* reg was read, so we're sensitive to initial value */ - REG_LIVE_WRITTEN, /* reg was written first, screening off later reads */ - REG_LIVE_DONE = 4, /* liveness won't be updating this register anymore */ -}; +/* Reg hasn't been read or written this branch. */ +#define REG_LIVE_NONE 0x0 +/* Reg was read, so we're sensitive to initial value. */ +#define REG_LIVE_READ 0x1 +/* Reg was written first, screening off later reads. */ +#define REG_LIVE_WRITTEN 0x2 +/* Liveness won't be updating this register anymore. */ +#define REG_LIVE_DONE 0x4 struct bpf_reg_state { /* Ordering of fields matters. See states_equal() */ @@ -131,7 +133,7 @@ struct bpf_reg_state { * pointing to bpf_func_state. */ u32 frameno; - enum bpf_reg_liveness live; + u8 live; }; enum bpf_stack_slot_type { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index dffeec3..6cc8c38 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -407,8 +407,7 @@ static char slot_type_char[] = { [STACK_ZERO] = '0', }; -static void print_liveness(struct bpf_verifier_env *env, - enum bpf_reg_liveness live) +static void print_liveness(struct bpf_verifier_env *env, u8 live) { if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN | REG_LIVE_DONE)) verbose(env, "_"); @@ -5687,8 +5686,8 @@ static bool check_ids(u32 old_id, u32 cur_id, struct idpair *idmap) static void clean_func_state(struct bpf_verifier_env *env, struct bpf_func_state *st) { - enum bpf_reg_liveness live; int i, j; + u8 live; for (i = 0; i < BPF_REG_FP; i++) { live = st->regs[i].live;
"enum bpf_reg_liveness" is actually used as bit instead of integer. For example: if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN | REG_LIVE_DONE)) Using enum to represent bit is error prone, better to use explicit bit macros. Signed-off-by: Jiong Wang <jiong.wang@netronome.com> --- include/linux/bpf_verifier.h | 16 +++++++++------- kernel/bpf/verifier.c | 5 ++--- 2 files changed, 11 insertions(+), 10 deletions(-)