Windows fio build processor group support testing

Windows fio build processor group support testing

[Sitsofe Wheeler]

I have a git tree (https://github.com/sitsofe/fio/commits/proc_group )
that adds processor group support to fio CPU setting but to do so it
introduces a flag to target the build to Windows 7 and sets it to be
the default. If this change goes in it will mean if you would need to
explicitly set a flag to have an fio that will work for
WinXP/Vista/2003/2008 (before R2) etc. If you have time, any feedback
would be useful.

If I don't hear any complaints I'll send a pull request to Jens in a day or two.

-- 
Sitsofe | http://sucs.org/~sits/

Re: Windows fio build processor group support testing

[Bart Van Assche]

On Tue, 2018-03-27 at 21:13 +0100, Sitsofe Wheeler wrote:
> I have a git tree (https://github.com/sitsofe/fio/commits/proc_group )
> that adds processor group support to fio CPU setting but to do so it
> introduces a flag to target the build to Windows 7 and sets it to be
> the default. If this change goes in it will mean if you would need to
> explicitly set a flag to have an fio that will work for
> WinXP/Vista/2003/2008 (before R2) etc. If you have time, any feedback
> would be useful.
> 
> If I don't hear any complaints I'll send a pull request to Jens in a day or two.

Hello Sitsofe,

My feedback about these patches is as follows:
- Having to specify a target Windows version at build time seems wrong to me.
  What I did myself when I wrote Windows code 15 years ago was to detect at
  runtime which functionality is available and which functionality is not
  available. An initialization function could e.g. call LoadLibrary() and
  GetProcAddress(). The function pointer(s) returned by GetProcAddress() can
  be stored in global variables. Any function that needs code that is not
  available on every supported Windows version can then check these function
  pointers, starting with the most recently introduced function, and call a
  function that is available on the Windows platform fio has been started on.
- Explicit XP / 2000 / etc. version checks seem unfortunate to me because I
  think these will lead to maintainability problems in the future.
- Your patch series adds a lot of code in header files which will slow down
  the build. I think such code should be added to .c files instead.

Thanks,

Bart.

Re: Windows fio build processor group support testing

[Sitsofe Wheeler]

Hi Bart,

On 27 March 2018 at 21:25, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> My feedback about these patches is as follows:
> - Having to specify a target Windows version at build time seems wrong to me.
>   What I did myself when I wrote Windows code 15 years ago was to detect at
>   runtime which functionality is available and which functionality is not
>   available. An initialization function could e.g. call LoadLibrary() and
>   GetProcAddress(). The function pointer(s) returned by GetProcAddress() can
>   be stored in global variables. Any function that needs code that is not
>   available on every supported Windows version can then check these function
>   pointers, starting with the most recently introduced function, and call a
>   function that is available on the Windows platform fio has been started on.

I'll note that Jens suggested we could use build time configuration in
https://github.com/axboe/fio/issues/527#issuecomment-363804643 . With
runtime lookup it becomes tricky to test the legacy paths on new
systems because you can no longer force them to be taken (I don't have
access to a Windows before 2012 R2 myself). My hope is that support
for legacy Windows is eventually dropped because:
- Most mainstream versions of Windows before Windows 7/Windows Server
2008 are already EOL (both versions of Server 2008 go EOL in 2020)
- It's unclear who is running fio is running fio in those old Windows
environments

I also suspect we've quietly dropped support for old versions of other
OSes due to build requirements - it looks like it would be a struggle
to build a recent fio for a RHEL 4 era system due compiler
requirements (and who knows if there are other problems when using an
older glibc).

> - Explicit XP / 2000 / etc. version checks seem unfortunate to me because I
>   think these will lead to maintainability problems in the future.

The fact the code has a conditional (either a compile one or a runtime
one) is what's unfortunate and there's a maintenance burden either way
due to increased paths. The only way to save maintenance effort would
be to drop the old path...

> - Your patch series adds a lot of code in header files which will slow down
>   the build. I think such code should be added to .c files instead.

This argument applies to the other bigger os/os-*.h files (e.g.
os/os-linux.h) too but I think making such a change (and ensuring the
pieces that are performance sensitive stay inlinable) could be done in
a separate patch (which would be able to introduce an os.o) in the
future.

-- 
Sitsofe | http://sucs.org/~sits/

Re: Windows fio build processor group support testing

[Bart Van Assche]

On Wed, 2018-03-28 at 11:48 +0100, Sitsofe Wheeler wrote:
> I'll note that Jens suggested we could use build time configuration in
> https://github.com/axboe/fio/issues/527#issuecomment-363804643 . With
> runtime lookup it becomes tricky to test the legacy paths on new
> systems because you can no longer force them to be taken (I don't have
> access to a Windows before 2012 R2 myself).

Let's follow what Jens proposed. But I do not agree that my proposal would
make it tricky to test the legacy paths. One possible approach would be to
add a command-line option that forces the legacy paths to be taken.

Bart.

Re: Windows fio build processor group support testing

[Jens Axboe]

On 3/28/18 8:20 AM, Bart Van Assche wrote:
> On Wed, 2018-03-28 at 11:48 +0100, Sitsofe Wheeler wrote:
>> I'll note that Jens suggested we could use build time configuration in
>> https://github.com/axboe/fio/issues/527#issuecomment-363804643 . With
>> runtime lookup it becomes tricky to test the legacy paths on new
>> systems because you can no longer force them to be taken (I don't have
>> access to a Windows before 2012 R2 myself).
> 
> Let's follow what Jens proposed. But I do not agree that my proposal would
> make it tricky to test the legacy paths. One possible approach would be to
> add a command-line option that forces the legacy paths to be taken.

I think I've done one overhaul of the cpu mask mechanism, to make it more
cross platform friendly. And that was many years ago. So I don't expect
a lot of churn in this area, which should mean that the legacy code can
remain mostly untouched going forward.

If, at some later point in time that changes, then I'm fine with ditching
the old Windows processor group code if we have no way of verifying
whether it still works or not.

Hence I think that Sitsofe's approach is fine - default to the code that
works well on newer Windows and supports > 32 CPUs. That's a basic
system these days, and what most people would run on.

-- 
Jens Axboe