Thread safety Part 1

[DennisHeimbigner]

Since changes to the netcdf-c library appear to be slowing down for the moment,
I am going to create a PR that is an initial step to thread safety support.
Specifically, I am going to start collecting all global variables and computed
constants into a single file.
There are some complexities.

1. Some global variables are used by all (or almost all) dispatchers.
   These will need to be protected by a global mutex.
2. Some global variables are used by only a single dispatcher.
   Thread safe access to them will need to be done by that dispatcher's code.
3. Computed constants will be initialized by a single initialization routine
   that uses a separate mutex. Once these constants are initialized, they will
   be accessible without any synchronization.

[WardF]

That sounds good; I'm going to push through the outstanding PR's as they stand at the moment, but once that's done we can wait for this :)

[DennisHeimbigner]

No need to hold up most merges. It is only really big ones that may cause trouble.

[edhartnett]

Dennis, I look forward to seeing this work develop.

Also, how do you plan to test?

[wkliao]

This will be a great feature in NetCDF. I am also looking forward to see it.

FYI. There is a test program in PnetCDF for testing one-file-per-thread safety.
https://github.com/Parallel-NetCDF/PnetCDF/blob/master/test/testcases/tst_pthread.c
Feel free to make use of it.

[DennisHeimbigner]

Thanks. What is the general state of thread support in pnetcdf?

[wkliao]

Starting from 1.11.0, PnetCDF supports one-file-per-thread safety.
It does not support multi-threading for accessing the same file.

[magnusuMET]

Over on the Rust wrapper (https://github.com/mhiley/rust-netcdf) we are wondering how unsafe the library is when multi-threading.

• Could we assume reading from the same file from two places simultaneously is safe?
• Is writing data to one file, and reading data from another safe?

[ruedigergad]

Over on the Rust wrapper (https://github.com/mhiley/rust-netcdf) we are wondering how unsafe the library is when multi-threading.

Hi,
I hope it is ok to reply here. I also came across the issue of multi-threading vs. NetCDF and just want to share some of the results I got, in the hope that they are helpful.

I understand this is about netcdf-c but for the sake of brevity, I use the following Python snippet for illustration purposes:

#!/usr/bin/env python3

from netCDF4 import Dataset
from threading import Thread

data_1 = Dataset('data_1.nc', 'w', format='NETCDF4')
data_1.createDimension('a', 4)
a = data_1.createVariable('a', int, ('a'))
a[:] = [1, 7, 0, 1]

data_2 = Dataset('data_2.nc', 'w', format='NETCDF4')
data_2.createDimension('b', 4)
b = data_2.createVariable('b', int, ('b'))
b[:] = [1, 8, 6, 4]

class MakeItSo(Thread):
    def __init__(self, dataset):
        super().__init__()
        self.dataset = dataset

    def run(self):
        while True:
            for v in self.dataset.variables:
                self.dataset[v][:] = self.dataset[v][:] + 1

m_1 = MakeItSo(data_1)
m_2 = MakeItSo(data_2)

m_1.start()
m_2.start()

With this minimum working example, I can reliably reproduce a segfault. So, even using two distinct datasets seems to be problematic.
However, the segfault happens not in netcdf-c but in libhdf5. See, e.g., the following gdb backtrace:

#0  0x00007fffe97b0b9a in H5CX_pop () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libhdf5-0f1e5d78.so.103.0.0
#1  0x00007fffe98e8a95 in H5Pcreate () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libhdf5-0f1e5d78.so.103.0.0
#2  0x00007fffe9fd5acf in NC4_put_vars () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libnetcdf-bb7fb3dc.so.15.0.0
#3  0x00007fffe9fd5335 in NC4_put_vara () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libnetcdf-bb7fb3dc.so.15.0.0
#4  0x00007fffe9f7a0ae in ?? () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libnetcdf-bb7fb3dc.so.15.0.0
#5  0x00007fffe9f7b339 in nc_put_vara () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/.libs/libnetcdf-bb7fb3dc.so.15.0.0
#6  0x00007fffea406c8c in ?? () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/_netCDF4.cpython-37m-x86_64-linux-gnu.so
#7  0x00007ffff7b99e7f in PyCFunction_Call () from /lib64/libpython3.7m.so.1.0
#8  0x00007fffea3ccab6 in ?? () from /home/rc/tmp/netcdf/env/lib64/python3.7/site-packages/netCDF4/_netCDF4.cpython-37m-x86_64-linux-gnu.so
...

Following this path, it shows that libhdf5 itself may not be thread-safe:
https://www.unidata.ucar.edu/mailing_lists/archives/netcdfgroup/2016/msg00223.html

Following the suggestion from this link and building libhdf5 with the following configure options makes the above minimum working example run without segfault:

./configure --enable-threadsafe --enable-hl --enable-unsupported

However, as stated in the libhdf5 documentation, "--enable-threadsafe" and "--enable-hl" are mutually exclusive and their combination is unsupported. Albeit, it seems to work at least for the minimum example above.

I hope this information is helpful.

Cheers,
Ruediger

[DennisHeimbigner]

There is (was?) an hdf5 build flag that serialized access to the hdf5 API.
So irrespective of netcdf-c, you need to build hdf5 with that flag set.
The most important problem with netcdf-c is that it keeps a global
list (actually more of a vector) that points to the main dtaa structure
for each open file. The danger is that this list can be corrupted
when two or more threads open files at the same time.
One possible hack is to modify your code to use a lock around
calls to nc_open, nc_create, nc_close, and nc_abort.

The better solution for netcdf-c is to analyze the code for non-constant
global data structures. I had been in the process of doing this, but
got sidetracked.

[magnusuMET]

When working on the wrapper for netcdf, we found it was easy to corrupt internal data structures when working with a non-threadsafe hdf5 install. In essence, every call to netcdf must be protected by the same mutex. In the rust-wrapper we ended up putting all calls to netcdf under a mutex, and discouraging using hdf5 simultaneously, although this leads to poor multithreaded performance.

A workaround for multi-threaded (read-only) access is direct file-access, as e.g. @gauteh implemented in hidefix.

[edwardhartnett]

It seems that this work must be done in each dispatch layer. For example, for netCDF/HDF5, in libsrc4 and libhdf5, we protect with a mutex each function that changes any internal data. Is that what you had in mind?

If we work together, you doing the classic and opendap, me doing HDF5 and HDF4, we could probably do that pretty quickly. And it would not hurt to do it incrementally (i.e. one dispatch layer at a time). Can we come up with a programming idiom for this? Do you have a first-case that you have explored? Presumably this is just going to be some macro that wraps the code to check the mutex, wait for it to be available, and then set it and go. And then another macro that unsets the mutex.

Then each function that changed the internal metadata would get something like:

CHECK_NETCDF_MUTEX
...
function code
...
FREE_NETCDF_MUTEX

Multi threaded operation would also be of great use to NOAA HPC codes, and, presumably, to those of other large data producers.

[DennisHeimbigner]

There are two ways to do this, I think. The simplest is to do what hdf5 did,
which was to serialize access to the netcdf library by wrapping
all calls in libdispatch with mutex.
The better, but harder solution is to wrap global
data access functions with mutexes. This latter, to be safe,
requires that we implement access functions (get/put)
to each global data item and then serialize access to those access functions.
A combination is also possible when we do not have access to all the global
data in other libraries (hdf5, hdf4, curl, etc).

[edwardhartnett]

What about my suggestion of protecting all functions that now change metadata?

Seems like that would be easier than get/set functions, and would allow multiple threads to read the metadata at one time, as would not be the case if we serialized access to the library.

Reads of metadata should be able to happen in multiple threads at the same time. Only if someone wants to change the value do we need to serialize access, right?

Let me be more specific. From nc4internal.h we can clearly see which functions just read metadata (find_nc_*()) and which change metadata.

/* Find items in the in-memory lists of metadata. */
int nc4_find_nc_grp_h5(int ncid, NC **nc, NC_GRP_INFO_T **grp,
                       NC_FILE_INFO_T **h5);
int nc4_find_grp_h5(int ncid, NC_GRP_INFO_T **grp, NC_FILE_INFO_T **h5);
int nc4_find_nc4_grp(int ncid, NC_GRP_INFO_T **grp);
int nc4_find_dim(NC_GRP_INFO_T *grp, int dimid, NC_DIM_INFO_T **dim,
                 NC_GRP_INFO_T **dim_grp);
int nc4_find_var(NC_GRP_INFO_T *grp, const char *name, NC_VAR_INFO_T **var);
int nc4_find_dim_len(NC_GRP_INFO_T *grp, int dimid, size_t **len);
int nc4_find_type(const NC_FILE_INFO_T *h5, int typeid1, NC_TYPE_INFO_T **type);
NC_TYPE_INFO_T *nc4_rec_find_named_type(NC_GRP_INFO_T *start_grp, char *name);
NC_TYPE_INFO_T *nc4_rec_find_equal_type(NC_GRP_INFO_T *start_grp, int ncid1,
                                        NC_TYPE_INFO_T *type);
int nc4_find_nc_att(int ncid, int varid, const char *name, int attnum,
                    NC_ATT_INFO_T **att);
int nc4_find_grp_h5_var(int ncid, int varid, NC_FILE_INFO_T **h5,
                        NC_GRP_INFO_T **grp, NC_VAR_INFO_T **var);
int nc4_find_grp_att(NC_GRP_INFO_T *grp, int varid, const char *name,
                     int attnum, NC_ATT_INFO_T **att);
int nc4_get_typeclass(const NC_FILE_INFO_T *h5, nc_type xtype,
                      int *type_class);

/* Free various types */
int nc4_type_free(NC_TYPE_INFO_T *type);

/* These list functions add and delete vars, atts. */
int nc4_nc4f_list_add(NC *nc, const char *path, int mode);
int nc4_nc4f_list_del(NC_FILE_INFO_T *h5);
int nc4_file_list_add(int ncid, const char *path, int mode,
                      void **dispatchdata);
int nc4_file_list_get(int ncid, char **path, int *mode,
                      void **dispatchdata);
int nc4_file_list_del(int ncid);

[DennisHeimbigner]

But you need to make sure that you can isolate all metadata accesses thru
some closed internal API. You need to do some significant code examination
to find all the accesses to the metadata. I assume that the goal is to
allow threaded access to the read/write data functions (nc_get/put_vars).
But you still need to verify that the get/put_vars code accesses the metadata
only using the well-defined metadata API. It basically requires some serious
code analysis, In effect you need to partition the code into two
completely disjoint parts. Do-able, but non-trivial. As an exercise, you might
attempt an analysis of the libhdf5 get/put vars code to see if it seems feasible.
Also note that this does not solve the non-thread-safe-hdf5 problem
since the get/put vars code uses HDF5 to read/write and we have no
idea what kind of shared data is being used by hdf5 under the hood.

[DennisHeimbigner]

Personally my guess is that locking the global data is better than locking the code.
We can do some name changing on the known global data to catch
all references.

[edwardhartnett]

Well if we lock the global data by struct instead, then the getter functions are the existing find_* functions, right?

[DennisHeimbigner]

Maybe, but I bet if you look, you find any number of places where structs
are accessed directly by field. These would all have to be changed to use
get/set functions.

[DennisHeimbigner]

Remember, it is not enough to lock modifications. Reads must also be locked.

[edwardhartnett]

If the file is opened read-only, do reads need to be locked?

[edwardhartnett]

Let me see if an example will help me understand. Here's some code from libhdf4:

static int
hdf4_read_att(NC_FILE_INFO_T *h5, NC_VAR_INFO_T *var, int a)
{
    NC_HDF4_FILE_INFO_T *hdf4_file;
    NC_ATT_INFO_T *att;
    NCindex *att_list;
    int32 att_data_type, att_count;
    size_t att_type_size;
    char name[NC_MAX_HDF4_NAME+1];
    int sd_id;
    int retval;

    LOG((3, "%s: a %d var %s", __func__, a, var ? var->hdr.name : "global"));

    /* Check inputs. */
    assert(h5 && h5->format_file_info);

    /* Get the HDF4 file info. */
    hdf4_file = h5->format_file_info;

    /* Decide what att list to use, global or from a var. */
    if (var)
    {
        NC_VAR_HDF4_INFO_T *hdf4_var;
        assert(var->format_var_info);
        att_list = var->att;
        hdf4_var = var->format_var_info;
        sd_id = hdf4_var->sdsid;
    } else {
        att_list = h5->root_grp->att;
        sd_id = hdf4_file->sdid;
    }
...

In this code we read various fields. In the assert we read h5->format_file_info.

A bit later on, we also read fields from the var struct, format_file_info, att, sdsid, etc.

You are saying that each of those must be accessed through a get function instead? So each can have its own mutex?

[gauteh]

Yes, chunks are read and decompressed to a non-thread safe (but size limited) cache. tir. 2. jun. 2020, 21:53 skrev Edward Hartnett <[email protected]>:

…

If the file is opened read-only, do reads need to be locked? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1373 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAN3636DA2MCFTQ2X2E57LRUVKC7ANCNFSM4HAAHU7Q> .

[DennisHeimbigner]

The short answer is yes. The basic problem is that if some other thread
is making changes to the meta-data, modifying or adding an attribute, say.
Then without detailed code analysis, you cannot know when the meta data
is in a consistent state. If another thread tries to read from an inconsistent
state, then it could get a wrong value.
In terms of the locks, you could use a single lock for each global data structure
or you could use a single lock for all global data structures. The problem with the
first is that a thread would need to acquire all the locks for the structures
it intends to modify. This has to be done carefully or you can get deadlocks.
Using a single lock for all global data structures reduces the amount of
parallelism, but guarantees no deadlock.

[edwardhartnett]

Well crap, that's a lot of work.

[DennisHeimbigner]

True. Also about read-only files. The problem there is lazy reading
of variables and attributes. If we could actually force reading of all
of the metadata at once rather than on-the-fly, then we could probably
avoid most locking because the state would not change. This should
be doable by some simple changes to our meta-data reading algorithms
to suppress lazy reading.

In any case, this is probably why HDF5 went for a single global lock
for the whole API. We could do the same, but it gives up all usable
parallelism. We should throw this open to more general
discussion. Perhaps others have suggestions for getting usable
thread-safety.

[DennisHeimbigner]

The more I think about it, the more I like the read-only file special case as a target.
It is probably a major per-cent of the access patterns that could use threading.
It has the cost of reading all the meta-data. Perhaps we could mitigate
this by adding an API flag that essentially means that the client
has touched all the metadata he needs and to prohibit lazy reading
of any more metadata. This could be done on a per-open-file basis.
Seems like a reasonable compromise point.

[edwardhartnett]

Certainly we can turn off lazy reading (that is, force a read of all metadata on file open) in select cases. This could be trivially done with an NC_MULTITHREADED flag, for example.

Reading with multithreading would indeed be worth having, even if it only worked for read-only files.

But I think the holy grail is going to be to have different threads write data at the same time. Metadata is not a big deal in terms of the total I/O time. What is most important is when they are writing all their data for a timestep. If different threads could write to the file, that would be great.

[DennisHeimbigner]

Good point. But as long as the metadata is read-only for the duration
of the I/O it would be ok. But one problem here is unlimited dimensions
since they and all variables that the same unlimited dim are potentially modified.

However, the gotcha still remains. We just do not have enough control
over HDF5 to ensure that its global state is thread-safe. I can see no way
around that.

[edwardhartnett]

Well this may be a case where the classic format pulls ahead. ;-)

[niziak]

Please see my comment: #1495 (comment)