Re: [Qemu-devel] [RFC PATCH 4/4] nvdimm acpi: build flush hint address structure if required

[Dan Williams]

On Thu, Apr 6, 2017 at 3:13 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Fri, Mar 31, 2017 at 04:41:47PM +0800, Haozhong Zhang wrote:
>>
>> Add an boolean option 'flush-hint' to device 'nvdimm'. If it's on, a
>> flush hint address structure will be constructed for each nvdimm
>> device.
>
> Users should not need to set the flush hint option.  NVDIMM
> configurations that persist data properly without Flush Hint Addresses
> shouldn't use them (for best performance).  Configurations that rely on
> flush hints *must* use them to guarantee data integrity.
>
> I don't remember if there's a way to detect the configuration from host
> userspace, but we should focus on setting this correctly without
> requiring users to know which setting is necessary.
>
>> Signed-off-by: Haozhong Zhang <address@hidden>
>> ---
>>  hw/acpi/nvdimm.c        | 106 
>> +++++++++++++++++++++++++++++++++++++++++++++---
>>  hw/i386/pc.c            |   5 ++-
>>  hw/mem/nvdimm.c         |  26 ++++++++++++
>>  include/hw/mem/nvdimm.h |   2 +-
>>  4 files changed, 132 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index ea2ac3e..a7ff0b2 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -32,6 +32,8 @@
>>  #include "hw/acpi/bios-linker-loader.h"
>>  #include "hw/nvram/fw_cfg.h"
>>  #include "hw/mem/nvdimm.h"
>> +#include "exec/address-spaces.h"
>> +#include "qapi/error.h"
>>
>>  static int nvdimm_device_list(Object *obj, void *opaque)
>>  {
>> @@ -168,6 +170,22 @@ struct NvdimmNfitControlRegion {
>>  typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
>>
>>  /*
>> + * NVDIMM Flush Hint Address Structure
>> + *
>> + * It enables the data durability mechanism via ACPI.
>> + */
>> +struct NvdimmNfitFlushHintAddress {
>> +    uint16_t type;
>> +    uint16_t length;
>> +    uint32_t nfit_handle;
>> +    uint16_t nr_flush_hint_addr;
>> +    uint8_t  reserved[6];
>> +#define NR_FLUSH_HINT_ADDR 1
>> +    uint64_t flush_hint_addr[NR_FLUSH_HINT_ADDR];
>
> How should the number of flush hints be set?  This patch hardcodes it to
> 1 but the Linux nvdimm drivers tries to balance between flush hint
> addresses to improve performance (to prevent cache line bouncing?).

No, 1 should be fine for qemu. The reason for multiple flush hints is
to accommodate multiple flush queues in the hardware. Since each flush
takes some amount of time you can get more parallelism  if multiple
threads are waiting for the same flush event, rather than being queued
serially. I don't think you can realize the same parallelism with
fsync() calls.