Received-SPF: pass (google.com: domain of linux-kernel+bounces-24389-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [PATCH bpf-next v2] selftests/bpf: Skip callback tests if jit is
 disabled in test_verifier
To: Hou Tao <houtao@huaweicloud.com>, Alexei Starovoitov <ast@kernel.org>,
 Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>
References: <20240112015700.19974-1-yangtiezhu@loongson.cn>
 <1e919c98-2fc4-bd03-df19-97c4e8a24649@huaweicloud.com>
Cc: Eduard Zingerman <eddyz87@gmail.com>,
 John Fastabend <john.fastabend@gmail.com>, Jiri Olsa <jolsa@kernel.org>,
 bpf@vger.kernel.org, linux-kernel@vger.kernel.org
From: Tiezhu Yang <yangtiezhu@loongson.cn>
Message-ID: <1930b1eb-afff-8509-e233-26c28e7622fd@loongson.cn>
Date: Fri, 12 Jan 2024 15:40:52 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <1e919c98-2fc4-bd03-df19-97c4e8a24649@huaweicloud.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit


On 01/12/2024 12:21 PM, Hou Tao wrote:
> Hi,
>
> On 1/12/2024 9:57 AM, Tiezhu Yang wrote:
>> If CONFIG_BPF_JIT_ALWAYS_ON is not set and bpf_jit_enable is 0, there
>> exist 6 failed tests.

..

>> +static bool is_jit_enabled(void)
>> +{
>> +	const char *jit_sysctl = "/proc/sys/net/core/bpf_jit_enable";
>> +	bool enabled = false;
>> +	int sysctl_fd;
>> +
>> +	sysctl_fd = open(jit_sysctl, 0, O_RDONLY);
>
> It should be open(jit_sysctl, O_RDONLY).

Yes, this function comes from test_progs.c, I think
it is better to move it to testing_helpers.c with
this change.

>> +	if (sysctl_fd != -1) {
>> +		char tmpc;
>> +
>> +		if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
>> +			enabled = (tmpc != '0');
>> +		close(sysctl_fd);
>> +	}
>> +
>> +	return enabled;
>> +}
>> +
>>  static int null_terminated_insn_len(struct bpf_insn *seq, int max_len)
>>  {
>>  	int i;
>> @@ -1662,6 +1691,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>>  		goto close_fds;
>>  	}
>>
>> +	if (!is_jit_enabled()) {
>
> Is it necessary to check whether jit is enabled or not each time ? Could
> we just check it only once just like unpriv_disabled does ?

Yes, it looks better, will modify the related code.

>> +		for (i = 0; i < prog_len; i++, prog++) {
>
> Is it better to only check pseudo_func only when both fd_prog < 0 and
> saved_errno == EINVAL are true, so unnecessary check can be skipped ?

Yes, will do it like this:

   if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled)

Thanks,
Tiezhu