> On 11 Apr 2019, at 17:44, Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx> wrote:
>
> On Thu, 11 Apr 2019 07:13:03 +0100, Jiong Wang wrote:
>>>> @@ -1150,17 +1150,17 @@ static int mark_reg_read(struct bpf_verifier_env *env,
>>>> parent->var_off.value, parent->off);
>>>> return -EFAULT;
>>>> }
>>>> - if (parent->live & REG_LIVE_READ)
>>>> + if ((parent->live & REG_LIVE_READ) == flags)
>>>> /* The parentage chain never changes and
>>>> - * this parent was already marked as LIVE_READ.
>>>> + * this parent was already marked with all read bits.
>>>
>>> Do we have to propagate all read bits? Read64 is strictly stronger
>>> than read32, as long as read64 is set on the parent we should be good?
>>
>> We should be good, but I doubt there is value to differentiate on this in this
>> kind of HOT function.
>
> The entire if clause is an optimization. I'm saying you can maintain it
> as more aggressive.
What I mean is I suspect the read width could be quite consistent in real program,
the percentage for doing extra check on read64 could actually be mishit for
most of the time, if the propagation iterations is big the extra check done each
time may overcome the propagation pruned.
I will do some benchmarking on this to see the real gain.
Regards,
Jiong
>
>>>> @@ -6196,12 +6286,19 @@ static int propagate_liveness_reg(struct bpf_verifier_env *env,
>>>> struct bpf_reg_state *reg,
>>>> struct bpf_reg_state *parent_reg)
>>>> {
>>>> + u8 parent_bits = parent_reg->live & REG_LIVE_READ;
>>>> + u8 bits = reg->live & REG_LIVE_READ;
>>>> + u8 bits_diff = parent_bits ^ bits;
>>>> + u8 bits_prop = bits_diff & bits;
>>>> int err;
>>>>
>>>> - if (parent_reg->live & REG_LIVE_READ || !(reg->live & REG_LIVE_READ))
>>>> + /* "reg" and "parent_reg" has the same read bits, or the bit doesn't
>>>> + * belong to "reg".
>>>> + */
>>>> + if (!bits_diff || !bits_prop)
>>>
>>> bits_prop is a subset of bits_diff, no? !bits_prop is always true
>>> if !bits_diff is true, no need to check both.
>>
>> Bits_prop is a subset of bits_diff WHEN it comes from “reg", we don’t want to
>> do the propagation when the diff comes from “parent_reg”, so, we need to check
>> both.
>
> Not sure what you're saying, in this patch:
>
> u8 bits_prop = bits_diff & bits;
>
> Maybe you're talking about some patch down the line..
