Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Apr 2023 19:01:44 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
cc:     linux-kselftest@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Subject: Re: [PATCH v2 10/24] selftests/resctrl: Split run_fill_buf() to
 alloc, work, and dealloc helpers
In-Reply-To: <8d6f94e6-4d51-5b96-2a74-2417ffcc0b51@intel.com>
Message-ID: <a38cba17-77c3-3f81-d4c9-fca839a46444@linux.intel.com>
References: <20230418114506.46788-1-ilpo.jarvinen@linux.intel.com> <20230418114506.46788-11-ilpo.jarvinen@linux.intel.com> <8d6f94e6-4d51-5b96-2a74-2417ffcc0b51@intel.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1840410181-1682352107=:2038"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1840410181-1682352107=:2038
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

On Fri, 21 Apr 2023, Reinette Chatre wrote:
> On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> > 
> > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> > index 5cdb421a2f6c..6f0438aa71a6 100644
> > --- a/tools/testing/selftests/resctrl/fill_buf.c
> > +++ b/tools/testing/selftests/resctrl/fill_buf.c
> > @@ -24,6 +24,11 @@
> >  
> >  static unsigned char *startptr;
> >  
> > +void free_buffer(void)
> > +{
> > +	free(startptr);
> > +}
> > +
> 
> >From what I understand startptr is a global variable because there used
> to be a signal handler that attempted to free the buffer as part of
> its cleanup. This was not necessary and this behavior no longer exists,
> yet the global buffer pointer remains.
> See commit 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler
> register/unregister for all tests")
> 
> I do not see why a global buffer pointer with all these indirections
> are needed. Why not just use a local pointer and pass it to functions
> as needed? In the above case, just call free(pointer) directly from the
> test.

OK, I'll try to convert all this into using non-global pointers then. It 
requires a bit refactoring but, IIRC, it is doable.

> >  static void sb(void)
> >  {
> >  #if defined(__i386) || defined(__x86_64)
> > @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
> >  	return 0;
> >  }
> >  
> > -static int
> > -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val)
> > +int alloc_buffer(unsigned long long buf_size, int memflush)
> >  {
> 
> This can be an allocation function that returns a pointer to
> allocated buffer, NULL if error.
>
> > -	unsigned char *start_ptr, *end_ptr;
> > -	int ret;
> > +	unsigned char *start_ptr;
> >  
> >  	start_ptr = malloc_and_init_memory(buf_size);
> >  	if (!start_ptr)
> >  		return -1;
> >  
> >  	startptr = start_ptr;
> > -	end_ptr = start_ptr + buf_size;
> >  
> >  	/* Flush the memory before using to avoid "cache hot pages" effect */
> >  	if (memflush)
> >  		mem_flush(start_ptr, buf_size);
> >  
> > +	return 0;
> > +}
> > +
> > +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val)
> > +{
> > +	unsigned char *end_ptr;
> > +	int ret;
> > +
> > +	end_ptr = startptr + buf_size;
> >  	if (op == 0)
> > -		ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
> > +		ret = fill_cache_read(startptr, end_ptr, resctrl_val);
> >  	else
> > -		ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
> > +		ret = fill_cache_write(startptr, end_ptr, resctrl_val);
> >  
> > -	if (ret) {
> > +	if (ret)
> >  		printf("\n Error in fill cache read/write...\n");
> > -		return -1;
> > -	}
> >  
> > -	free(startptr);
> > +	return ret;
> > +}
> >  
> 
> This seems like an unnecessary level of abstraction to me. Could
> callers not just call fill_cache_read()/fill_cache_write() directly?
> I think doing so will make tests easier to understand. Looking ahead
> at how cat_val() turns out in the final patch I do think a call
> to fill_cache_read() is easier to follow than this abstraction.

Passing a custom benchmark command with -b would lose some functionality 
if this abstraction is removed. CAT test could make a direct call though 
as it doesn't care about the benchmark command.

How useful that -b functionality is for selftesting is somewhat 
questionable though.


-- 
 i.

--8323329-1840410181-1682352107=:2038--