memkind hmat example

[bratpiorka]

This change is 

[maciekpac]

I think it would be best to check the naming, but this is only a suggestion.

[pbalcer]

formatting is a bit broken (mostly trailing whitespaces). Please take a look at PMDK coding style guidelines:
https://github.com/pmem/pmdk/blob/master/CODING_STYLE.md

You can also use cstyle (in PMDK repo) to check your code.

Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status: 1 of 7 files reviewed, 8 unresolved discussions (waiting on @bratpiorka and @pbalcer)

---

examples/I/hashmap.h, line 5 at r1 (raw file):

struct hashmap;
struct hashmap_keyval;
typedef void* value;

this is just confusing - just use void * directly inline.

---

examples/I/hashmap.h, line 12 at r1 (raw file):

};

struct hashmap* hashmap_create(int size, struct hashmap_alloc alloc);

wouldn't it be easier to just accept a memkind_t here?

Per convention we have in PMDK, this function should be called hashmap_new.

(you will notice in POSIX 'create/destroy' are used to instantiate an instance of something in an existing memory location - e.g., pthread_mutex_create)

---

examples/I/hashmap.h, line 13 at r1 (raw file):

struct hashmap* hashmap_create(int size, struct hashmap_alloc alloc);
void hashmap_destroy(struct hashmap* hashmap);

hashmap_delete

---

examples/I/hashmap.h, line 15 at r1 (raw file):

void hashmap_destroy(struct hashmap* hashmap);

struct hashmap_keyval* hashmap_set(struct hashmap* hashmap, char* key,

key length should be explicit

---

examples/I/hashmap.c, line 9 at r1 (raw file):

    int size;
    struct hashmap_alloc alloc;
    struct hashmap_keyval** buckets;

use flexible array member and allocate buckets together with the hashmap.

---

examples/I/hashmap.c, line 58 at r1 (raw file):

    int i;
    for (i = 0; i < hashmap->size; i++) {
        struct hashmap_keyval* iter = hashmap->buckets[i];

for (struct hashmap_keyval *iter = hashmap->buckets[i]; iter != NULL; iter = iter->next) {
  free(iter);
}

---

examples/I/hashmap.c, line 95 at r1 (raw file):

    while (*iter) {
        /* Check if the given key exists */
        if ((strlen((*iter)->key) == strlen(key)) && 

I don't think you need strlen here. You are essentially comparing iterating over each key twice.

---

examples/I/hashmap.c, line 124 at r1 (raw file):

    while (iter) {
        /* Check if the given key exists */
        if ((strlen(iter->key) == strlen(key)) && 

ditto

[bratpiorka]

Reviewable status: 1 of 7 files reviewed, 8 unresolved discussions (waiting on @pbalcer)

---

examples/I/hashmap.h, line 5 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

this is just confusing - just use void * directly inline.

Done.

---

examples/I/hashmap.h, line 12 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

wouldn't it be easier to just accept a memkind_t here?

Per convention we have in PMDK, this function should be called hashmap_new.

(you will notice in POSIX 'create/destroy' are used to instantiate an instance of something in an existing memory location - e.g., pthread_mutex_create)

Done.

---

examples/I/hashmap.h, line 13 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

hashmap_delete

Done.

---

examples/I/hashmap.h, line 15 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

key length should be explicit

Done.

---

examples/I/hashmap.c, line 9 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

use flexible array member and allocate buckets together with the hashmap.

Done.

---

examples/I/hashmap.c, line 58 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

for (struct hashmap_keyval *iter = hashmap->buckets[i]; iter != NULL; iter = iter->next) {
  free(iter);
}

Done.

---

examples/I/hashmap.c, line 95 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I don't think you need strlen here. You are essentially comparing iterating over each key twice.

Done.

---

examples/I/hashmap.c, line 124 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

ditto

Done.

[bratpiorka]

Reviewed 4 of 7 files at r1, 3 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pbalcer)

[pbalcer]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bratpiorka and @pbalcer)

---

examples/I/hashmap.c, line 22 at r4 (raw file):

static int hash_func(char* key)
{
    int len = strlen(key);

use the provided key length here.

---

examples/I/hashmap.c, line 108 at r4 (raw file):

}

void* hashmap_get(struct hashmap* hashmap, char* key)

this will need key length.

[bratpiorka]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pbalcer)

---

examples/I/hashmap.c, line 22 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

use the provided key length here.

Done.

---

examples/I/hashmap.c, line 108 at r4 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

this will need key length.

Done.

[maciekpac]

Reviewed 4 of 7 files at r1, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pbalcer)

[pbalcer]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bratpiorka)

---

examples/I/README.txt, line 4 at r5 (raw file):

Starting from Linux Kernel version 5.5, the NUMA nodes concept was enhanced with
new memory attributes: latency, capacity and bandwidth. We could use them to

we can use them?

---

examples/I/README.txt, line 5 at r5 (raw file):

Starting from Linux Kernel version 5.5, the NUMA nodes concept was enhanced with
new memory attributes: latency, capacity and bandwidth. We could use them to
allocate data used in different scenarios on different types of memory. Note,

In heterogeneous memory systems, where there are many, potentially varied, memory NUMA nodes, applications can use these new attributes to dynamically differentiate the type of memory it requests depending on the expected access patterns of the allocated data.

---

examples/I/README.txt, line 6 at r5 (raw file):

new memory attributes: latency, capacity and bandwidth. We could use them to
allocate data used in different scenarios on different types of memory. Note,
that we don't explicitly say which kind of medium (e.g. DRAM or PMEM) we want to

Note that in this example, we don't explicitly ...

---

examples/I/README.txt, line 8 at r5 (raw file):

that we don't explicitly say which kind of medium (e.g. DRAM or PMEM) we want to
use here, because some characteristics (like latency or bandwidth) could be
relative to relationship between source and destination NUMA nodes. Instead of

I think that would be difficult to grok for most people. And it's not the primary reason. Using the characteristics-based API is better because it's more flexible w.r.t. to the platform that the application is running on. It will work regardless of whether the server has PMem/HBM or whatever.

---

examples/I/README.txt, line 11 at r5 (raw file):

that we use a concept of "lowest latency" or "highest bandwidth" nodes that
could be accessed from a CPU that runs our program. This concept was added to
Memkind 1.11. To use it, a hwloc library must be installed on the system with

Is that a hard requirement? Does memkind just fail if it's not available? If it fails, then we need to add a fallback if hwloc/HMAT is not available.

---

examples/I/README.txt, line 18 at r5 (raw file):

memory kinds to allocate simple array of data and access it through a hashmap.
Because all hashmap operations like searching or adding new data should be
efficient, keys that identifies user data entries are allocated in lowest

keys that identify

---

examples/I/README.txt, line 20 at r5 (raw file):

efficient, keys that identifies user data entries are allocated in lowest
latency memory. On the other hand, because size of the data entires could be
huge, to allocate them we want a memory with highest capacity.

want memory with the highest

[bratpiorka]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pbalcer)

---

examples/I/README.txt, line 4 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

we can use them?

Done.

---

examples/I/README.txt, line 5 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

In heterogeneous memory systems, where there are many, potentially varied, memory NUMA nodes, applications can use these new attributes to dynamically differentiate the type of memory it requests depending on the expected access patterns of the allocated data.

I'm not sure which part of this paragraph I should replace with your suggestion.

---

examples/I/README.txt, line 6 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Note that in this example, we don't explicitly ...

Done.

---

examples/I/README.txt, line 8 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

I think that would be difficult to grok for most people. And it's not the primary reason. Using the characteristics-based API is better because it's more flexible w.r.t. to the platform that the application is running on. It will work regardless of whether the server has PMem/HBM or whatever.

I have no idea how I can rephrase this paragraph.

---

examples/I/README.txt, line 11 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Is that a hard requirement? Does memkind just fail if it's not available? If it fails, then we need to add a fallback if hwloc/HMAT is not available.

yes - this is described in Memkind README: "To get locality characteristics of NUMA nodes in the system you need hwloc-devel v2.3.0 (or later)."

In case libhwloc is not present, Memkind will fail with error:
"MEMKIND_ERROR: Memory attribute NUMA nodes cannot be automatically detected."

---

examples/I/README.txt, line 18 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

keys that identify

Done.

---

examples/I/README.txt, line 20 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

want memory with the highest

Done.

[pbalcer]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bratpiorka)

---

examples/I/README.txt, line 5 at r5 (raw file):

Previously, bratpiorka (Rafał Rudnicki) wrote…

I'm not sure which part of this paragraph I should replace with your suggestion.

Starting from "We can use them".

---

examples/I/README.txt, line 8 at r5 (raw file):

Previously, bratpiorka (Rafał Rudnicki) wrote…

I have no idea how I can rephrase this paragraph.

we want to use here, because typically the execution environment of applications can vary and might not have a specific memory type.

maybe something like that?

---

examples/I/README.txt, line 11 at r5 (raw file):

Previously, bratpiorka (Rafał Rudnicki) wrote…

yes - this is described in Memkind README: "To get locality characteristics of NUMA nodes in the system you need hwloc-devel v2.3.0 (or later)."

In case libhwloc is not present, Memkind will fail with error:
"MEMKIND_ERROR: Memory attribute NUMA nodes cannot be automatically detected."

that's not great - let's add a backlog item to address this with some fallback option.

[bratpiorka]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pbalcer)

---

examples/I/README.txt, line 5 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

Starting from "We can use them".

Done.

---

examples/I/README.txt, line 8 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

we want to use here, because typically the execution environment of applications can vary and might not have a specific memory type.

maybe something like that?

Done.

---

examples/I/README.txt, line 11 at r5 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

that's not great - let's add a backlog item to address this with some fallback option.

added - AESC-178

[pbalcer]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bratpiorka)

[lukaszstolarczuk]

@bratpiorka, if you want to continue work with this PR, please move it to the new base repository: https://github.com/pmemhackathon/hackathon

also, I believe it requires some Web content, to be displayed for this example (to be added on repo: https://github.com/pmemhackathon/hackathon.web)