Re: Threaded file system benchmark

[Ryo ONODERA <ryo_on%yk.rim.or.jp@localhost>]

Hi,

From: Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>, Date: Sun, 12 Jul 2015 15:13:56 +0100

> Ryo ONODERA <ryo_on%yk.rim.or.jp@localhost> wrote:
>> ...
>> 
>> --- a/gettime.c
>> +++ b/gettime.c
>> @@ -483,18 +483,30 @@ static void *clock_thread_fn(void *data)
>>  {
>>  	struct clock_thread *t = data;
>>  	struct clock_entry *c;
>> +#if defined(__NetBSD__)
>> +	os_cpu_mask_t *cpu_mask = cpuset_create();
>> +#else
>>  	os_cpu_mask_t cpu_mask;
>> +#endif
> 
> You can always use a pointer, e.g.:
> 
> #if defined(__NetBSD__)
> 	os_cpu_mask_t *cpu_mask = cpuset_create();
> #else
> 	os_cpu_mask_t cpu_mask_store;
> 	os_cpu_mask_t *cpu_mask = &cpu_mask_store;
> #endif
> 
> This will save some #ifdef __NetBSD__ elsewhere.

I will reduce #ifdef __NetBSD__.

>> --- a/hash.h
>> +++ b/hash.h
>> @@ -18,6 +18,10 @@
>>   * machines where multiplications are slow.
>>   */
>>  
>> +#if !defined(BITS_PER_LONG)
>> +#define	BITS_PER_LONG __SIZEOF_LONG__*8
>> +#endif
> 
> I did not look how exactly is this used, but it is a good idea to use
> brackets around the expression when dealing with macros.  Just prevents
> from any future bugs.

I will use "(__SIZEOF_LONG__*CHAR_BIT)".

>> +static inline int fio_setaffinity(int tid, os_cpu_mask_t *cpumask)
>> +{
>> +	return _sched_getaffinity(getpid(), tid, cpuset_size(cpumask), cpumask);
>> +}
> 
> I suppose you meant _sched_setaffinity() here?  Well, that is your bug.
> Also, _sched_{get,set}affinity() are internal calls which should not be
> used as a public API.  Instead, pthread_{get,set}affinity_np() should
> be used.  See affinity(3) man page for the details.

It may be the real cause of my problem.
Thank you very much.

I understand that internal calls should not be used.
I read affinity(3) man page, and found Solaris/HP-UX compatible pset(3)
APIs. 
I will use pset(3), I may borrow the code from Solaris's definitions.

>> --- a/parse.c
>> +++ b/parse.c
>> @@ -152,7 +152,7 @@ static unsigned long long get_mult_time(const char
>> *str, int len, 
>>  	c = strdup(p);
>>  	for (int i = 0; i < strlen(c); i++)
>> -		c[i] = tolower(c[i]);
>> +		c[i] = tolower((int)c[i]);
> 
> The value should be type casted to unsigned char rather than int.
> See CAVEATS section of the ctype(3) man page for the explanation.

I see.
I have added this to pass "gcc -Werror", however fio does not require
-Werror originally. I will remove it.

> -- 
> Mindaugas

Thank you very much.

--
Ryo ONODERA // ryo_on%yk.rim.or.jp@localhost
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB  FD1B F404 27FA C7D1 15F3