Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update compact dataset I/O routines to handle driver-level memory copy #1054

Merged
merged 3 commits into from
Sep 30, 2021
Merged

Update compact dataset I/O routines to handle driver-level memory copy #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea6174c
Update compact dataset I/O routines to handle driver-level memory copy
jhendersonHDF Sep 28, 2021
626d651
Combine compact I/O read/write struct into single I/O struct
jhendersonHDF Sep 29, 2021
c3fa7ba
Add RELEASE.txt entry for new "ctl" memory copy operation
jhendersonHDF Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,48 @@ New Features

Library:
--------
- Adds new file driver-level memory copy operation for
"ctl" callback and updates compact dataset I/O routines
to utilize it

When accessing an HDF5 file with a file driver that uses
memory allocated in special ways (e.g., without standard
library's `malloc`), a crash could be observed when HDF5
tries to perform `memcpy` operations on such a memory
region.

These changes add a new H5FD_FEAT_MEMMANAGE VFD feature
flag, which, if specified as supported by a VFD, will
inform HDF5 that the VFD either uses special memory
management routines or wishes to perform memory management
in a specific way. Therefore, this flag instructs HDF5 to
ask the file driver to perform memory management for
certain operations.

These changes also introduce a new "ctl" callback
operation identified by the H5FD_CTL__MEM_COPY op code.
This operation simply asks a VFD to perform a memory copy.
The arguments to this operation are passed to the "ctl"
callback's "input" parameter as a pointer to a struct
defined as:

struct H5FD_ctl_memcpy_args_t {
void * dstbuf; /**< Destination buffer */
hsize_t dst_off; /**< Offset within destination buffer */
const void *srcbuf; /**< Source buffer */
hsize_t src_off; /**< Offset within source buffer */
size_t len; /**< Length of data to copy from source buffer */
} H5FD_ctl_memcpy_args_t;

Further, HDF5's compact dataset I/O routines were
identified as a problematic area that could cause a crash
for VFDs that make use of special memory management. Those
I/O routines were therefore updated to make use of this new
"ctl" callback operation in order to ask the underlying
file driver to correctly handle memory copies.

(JTH - 2021/09/28)

- Adds new "ctl" callback to VFD H5FD_class_t structure
with the following prototype:

Expand Down
106 changes: 96 additions & 10 deletions src/H5Dcompact.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@
/* Local Typedefs */
/******************/

/* Callback info for I/O operation when file driver
* wishes to do its own memory management
*/
typedef struct H5D_compact_iovv_memmanage_ud_t {
H5F_shared_t *f_sh; /* Shared file for dataset */
void * dstbuf; /* Pointer to buffer to be read into/written into */
const void * srcbuf; /* Pointer to buffer to be read from/written from */
} H5D_compact_iovv_memmanage_ud_t;

/********************/
/* Local Prototypes */
/********************/
Expand All @@ -57,6 +66,7 @@ static hbool_t H5D__compact_is_space_alloc(const H5O_storage_t *storage);
static herr_t H5D__compact_io_init(const H5D_io_info_t *io_info, const H5D_type_info_t *type_info,
hsize_t nelmts, const H5S_t *file_space, const H5S_t *mem_space,
H5D_chunk_map_t *cm);
static herr_t H5D__compact_iovv_memmanage_cb(hsize_t dst_off, hsize_t src_off, size_t len, void *_udata);
static ssize_t H5D__compact_readvv(const H5D_io_info_t *io_info, size_t dset_max_nseq, size_t *dset_curr_seq,
size_t dset_size_arr[], hsize_t dset_offset_arr[], size_t mem_max_nseq,
size_t *mem_curr_seq, size_t mem_size_arr[], hsize_t mem_offset_arr[]);
Expand Down Expand Up @@ -239,6 +249,48 @@ H5D__compact_io_init(const H5D_io_info_t *io_info, const H5D_type_info_t H5_ATTR
FUNC_LEAVE_NOAPI(SUCCEED)
} /* end H5D__compact_io_init() */

/*-------------------------------------------------------------------------
* Function: H5D__compact_iovv_memmanage_cb
*
* Purpose: Callback operator for H5D__compact_readvv()/_writevv() to
* send a memory copy request to the underlying file driver.
*
* Return: Non-negative on success/Negative on failure
*
*-------------------------------------------------------------------------
*/
static herr_t
H5D__compact_iovv_memmanage_cb(hsize_t dst_off, hsize_t src_off, size_t len, void *_udata)
{
H5D_compact_iovv_memmanage_ud_t *udata = (H5D_compact_iovv_memmanage_ud_t *)_udata;
H5FD_ctl_memcpy_args_t op_args;
uint64_t op_flags;
H5FD_t * file_handle = NULL;
herr_t ret_value = SUCCEED;

FUNC_ENTER_STATIC

/* Retrieve pointer to file driver structure for ctl call */
if (H5F_shared_get_file_driver(udata->f_sh, &file_handle) < 0)
HGOTO_ERROR(H5E_IO, H5E_CANTGET, FAIL, "can't get file handle")

/* Setup operation flags and arguments */
op_flags = H5FD_CTL__ROUTE_TO_TERMINAL_VFD_FLAG | H5FD_CTL__FAIL_IF_UNKNOWN_FLAG;

op_args.dstbuf = udata->dstbuf;
op_args.dst_off = dst_off;
op_args.srcbuf = udata->srcbuf;
op_args.src_off = src_off;
op_args.len = len;

/* Make request to file driver */
if (H5FD_ctl(file_handle, H5FD_CTL__MEM_COPY, op_flags, &op_args, NULL) < 0)
HGOTO_ERROR(H5E_IO, H5E_FCNTL, FAIL, "VFD memcpy request failed")

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5D__compact_iovv_memmanage_cb() */

/*-------------------------------------------------------------------------
* Function: H5D__compact_readvv
*
Expand Down Expand Up @@ -268,11 +320,28 @@ H5D__compact_readvv(const H5D_io_info_t *io_info, size_t dset_max_nseq, size_t *

HDassert(io_info);

/* Use the vectorized memory copy routine to do actual work */
if ((ret_value = H5VM_memcpyvv(io_info->u.rbuf, mem_max_nseq, mem_curr_seq, mem_size_arr, mem_offset_arr,
io_info->store->compact.buf, dset_max_nseq, dset_curr_seq, dset_size_arr,
dset_offset_arr)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
/* Check if file driver wishes to do its own memory management */
if (H5F_SHARED_HAS_FEATURE(io_info->f_sh, H5FD_FEAT_MEMMANAGE)) {
H5D_compact_iovv_memmanage_ud_t udata;

/* Set up udata for memory copy operation */
udata.f_sh = io_info->f_sh;
udata.dstbuf = io_info->u.rbuf;
udata.srcbuf = io_info->store->compact.buf;

/* Request that file driver does the memory copy */
if ((ret_value = H5VM_opvv(mem_max_nseq, mem_curr_seq, mem_size_arr, mem_offset_arr, dset_max_nseq,
dset_curr_seq, dset_size_arr, dset_offset_arr,
H5D__compact_iovv_memmanage_cb, &udata)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
}
else {
/* Use the vectorized memory copy routine to do actual work */
if ((ret_value = H5VM_memcpyvv(io_info->u.rbuf, mem_max_nseq, mem_curr_seq, mem_size_arr,
mem_offset_arr, io_info->store->compact.buf, dset_max_nseq,
dset_curr_seq, dset_size_arr, dset_offset_arr)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
}

done:
FUNC_LEAVE_NOAPI(ret_value)
Expand Down Expand Up @@ -310,11 +379,28 @@ H5D__compact_writevv(const H5D_io_info_t *io_info, size_t dset_max_nseq, size_t

HDassert(io_info);

/* Use the vectorized memory copy routine to do actual work */
if ((ret_value = H5VM_memcpyvv(io_info->store->compact.buf, dset_max_nseq, dset_curr_seq, dset_size_arr,
dset_offset_arr, io_info->u.wbuf, mem_max_nseq, mem_curr_seq, mem_size_arr,
mem_offset_arr)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
/* Check if file driver wishes to do its own memory management */
if (H5F_SHARED_HAS_FEATURE(io_info->f_sh, H5FD_FEAT_MEMMANAGE)) {
H5D_compact_iovv_memmanage_ud_t udata;

/* Set up udata for memory copy operation */
udata.f_sh = io_info->f_sh;
udata.dstbuf = io_info->store->compact.buf;
udata.srcbuf = io_info->u.wbuf;

/* Request that file driver does the memory copy */
if ((ret_value = H5VM_opvv(dset_max_nseq, dset_curr_seq, dset_size_arr, dset_offset_arr, mem_max_nseq,
mem_curr_seq, mem_size_arr, mem_offset_arr, H5D__compact_iovv_memmanage_cb,
&udata)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
}
else {
/* Use the vectorized memory copy routine to do actual work */
if ((ret_value = H5VM_memcpyvv(io_info->store->compact.buf, dset_max_nseq, dset_curr_seq,
dset_size_arr, dset_offset_arr, io_info->u.wbuf, mem_max_nseq,
mem_curr_seq, mem_size_arr, mem_offset_arr)) < 0)
HGOTO_ERROR(H5E_IO, H5E_WRITEERROR, FAIL, "vectorized memcpy failed")
}

/* Mark the compact dataset's buffer as dirty */
*io_info->store->compact.dirty = TRUE;
Expand Down
24 changes: 24 additions & 0 deletions src/H5FDpublic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,14 @@
* enabled may be used as the Write-Only (W/O) channel driver.
*/
#define H5FD_FEAT_DEFAULT_VFD_COMPATIBLE 0x00008000
/*
* Defining H5FD_FEAT_MEMMANAGE for a VFL driver means that
* the driver uses special memory management routines or wishes
* to do memory management in a specific manner. Therefore, HDF5
* should request that the driver handle any memory management
* operations when appropriate.
*/
#define H5FD_FEAT_MEMMANAGE 0x00010000

/* ctl function definitions: */
#define H5FD_CTL_OPC_RESERVED 512 /* Opcodes below this value are reserved for library use */
Expand All @@ -185,6 +193,9 @@
#define H5FD_CTL__GET_MPI_COMMUNICATOR_OPCODE 2
#define H5FD_CTL__GET_MPI_RANK_OPCODE 3
#define H5FD_CTL__GET_MPI_SIZE_OPCODE 4
#define H5FD_CTL__MEM_ALLOC 5
#define H5FD_CTL__MEM_FREE 6
#define H5FD_CTL__MEM_COPY 7
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I only reserved the op codes for driver-level memory allocations and frees. I haven't added any argument structs for them since there isn't a use case yet, but can do so if it's useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree -- wait until we have use cases.


/* ctl function flags: */

Expand Down Expand Up @@ -367,6 +378,19 @@ typedef struct {
} H5FD_file_image_callbacks_t;
//! <!-- [H5FD_file_image_callbacks_t_snip] -->

/**
* Define structure to hold "ctl memory copy" parameters
*/
//! <!-- [H5FD_ctl_memcpy_args_t_snip] -->
typedef struct H5FD_ctl_memcpy_args_t {
void * dstbuf; /**< Destination buffer */
hsize_t dst_off; /**< Offset within destination buffer */
const void *srcbuf; /**< Source buffer */
hsize_t src_off; /**< Offset within source buffer */
size_t len; /**< Length of data to copy from source buffer */
} H5FD_ctl_memcpy_args_t;
//! <!-- [H5FD_ctl_memcpy_args_t_snip] -->

/********************/
/* Public Variables */
/********************/
Expand Down
5 changes: 2 additions & 3 deletions src/H5Fprivate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ typedef struct H5F_t H5F_t;
/* Include package's public header */
#include "H5Fpublic.h"

/* Public headers needed by this file */
#include "H5FDpublic.h" /* File drivers */

/* Private headers needed by this file */
#include "H5MMprivate.h" /* Memory management */
#include "H5FDprivate.h" /* File drivers */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since H5F_shared_get_file_handles needs the definition for the H5FD_t type, and since H5FDprivate.h includes H5FDpublic.h, just include the private header rather than both here.

#ifdef H5_HAVE_PARALLEL
#include "H5Pprivate.h" /* Property lists */
#endif /* H5_HAVE_PARALLEL */
Expand Down Expand Up @@ -906,6 +904,7 @@ H5_DLL hbool_t H5F_shared_has_feature(const H5F_shared_t *f, unsigned feature);
H5_DLL hbool_t H5F_has_feature(const H5F_t *f, unsigned feature);
H5_DLL haddr_t H5F_shared_get_eoa(const H5F_shared_t *f_sh, H5FD_mem_t type);
H5_DLL haddr_t H5F_get_eoa(const H5F_t *f, H5FD_mem_t type);
H5_DLL herr_t H5F_shared_get_file_driver(const H5F_shared_t *f_sh, H5FD_t **file_handle);
H5_DLL herr_t H5F_get_vfd_handle(const H5F_t *file, hid_t fapl, void **file_handle);

/* File mounting routines */
Expand Down
23 changes: 23 additions & 0 deletions src/H5Fquery.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,29 @@ H5F_get_eoa(const H5F_t *f, H5FD_mem_t type)
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5F_get_eoa() */

/*-------------------------------------------------------------------------
* Function: H5F_shared_get_file_driver
*
* Purpose: Returns a pointer to the file driver structure of the
* file's 'shared' structure.
*
* Return: file handle on success/abort on failure (shouldn't fail)
*-------------------------------------------------------------------------
*/
herr_t
H5F_shared_get_file_driver(const H5F_shared_t *f_sh, H5FD_t **file_handle)
{
/* Use FUNC_ENTER_NOAPI_NOINIT_NOERR here to avoid performance issues */
FUNC_ENTER_NOAPI_NOINIT_NOERR

HDassert(f_sh);
HDassert(file_handle);

*file_handle = f_sh->lf;

FUNC_LEAVE_NOAPI(SUCCEED)
} /* end H5F_shared_get_file_driver() */

/*-------------------------------------------------------------------------
* Function: H5F_get_vfd_handle
*
Expand Down