| Message ID | 152150305678.47105.9095916907704193253.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 19, 2018 at 04:45:30PM -0700, Dave Jiang wrote: > Adding helper functions to iterate through sysfs region persistence domain > attribute. The region will display the domain with the most persistence for the > region. The bus will display the domain attribute with the least persistence > amongst all the regions. ndctl_bus_get_persistence_domain() and > ndctl_region_get_persistence_domain are exported. ndctl list will also display > the region persistence domain as well. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Nice this is cleaner, and you've shaved off 20 lines. :) > @@ -755,6 +756,7 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > list_head_init(&bus->regions); > bus->ctx = ctx; > bus->id = id; > + bus->persistence_domain = PERSISTENCE_UNKNOWN; > > sprintf(path, "%s/dev", ctl_base); > if (sysfs_read_attr(ctx, path, buf) < 0 > @@ -916,6 +918,17 @@ NDCTL_EXPORT struct ndctl_bus *ndctl_bus_get_by_provider(struct ndctl_ctx *ctx, > return NULL; > } > > +NDCTL_EXPORT unsigned int Can we make this return an enum ndctl_persistence, like the internal functions do? This follows the lead of APIs like ndctl_namespace_get_mode(), which by returning an enum ndctl_namespace_mode tells the user what values to expect and what those values mean. Ditto for the ndctl_region_get_persistence_domain(), and the two external declarations in libndctl.h. > +ndctl_bus_get_persistence_domain(struct ndctl_bus *bus) > +{ > + struct ndctl_region *region; > + > + /* iterate through region to get the region persistence domain */ > + ndctl_region_foreach(bus, region) {} I still don't understand why you need to do a ndctl_region_foreach() here, looping through all the regions on a bus? I think you're just after the initialization that ndctl_region_foreach() ndctl_region_get_first() regions_init() gives you, right? If so, let's be more clear about that and just call regions_init() directly. This also saves the work of needlessly looping through the regions, and makes it so you don't need a local 'region' variable. > + > + return bus->persistence_domain; > +} > + > NDCTL_EXPORT struct ndctl_btt *ndctl_region_get_btt_seed(struct ndctl_region *region) > { > struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); > @@ -1755,6 +1768,62 @@ static int region_set_type(struct ndctl_region *region, char *path) > return 0; > } > > +static enum ndctl_persistence region_get_pd_type(char *name) > +{ > + if (strncmp("cpu_cache", name, 9) == 0) > + return PERSISTENCE_CPU_CACHE; > + else if (strncmp("memory_controller", name, 17) == 0) > + return PERSISTENCE_MEM_CTRL; > + else > + return PERSISTENCE_UNKNOWN; > +} > + > +static int region_persistence_scan(struct ndctl_region *region) > +{ > + struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); > + char *pd_path; > + FILE *pf; > + char buf[64]; > + int rc = 0; > + enum ndctl_persistence pd = PERSISTENCE_NONE; > + > + if (asprintf(&pd_path, "%s/persistence_domain", > + region->region_path) < 0) { > + rc = -errno; > + err(ctx, "region persist domain path allocation failure\n"); > + return rc; > + } > + > + pf = fopen(pd_path, "re"); > + if (!pf) { > + rc = -errno; > + free(pd_path); > + return rc; > + } > + > + region->persistence_domain = PERSISTENCE_NONE; I think that this initialization needs to happen at the top of the function before we have any return paths. This is because the code that calls this, add_region(), doesn't set region->persistence_domain if we give an error return. I think that this means that if the asprintf() fails, for example, we will end up with region->persistence_domain being uninitialized. > + do { > + rc = fscanf(pf, "%s", buf); > + if (rc == EOF) { > + if (ferror(pf)) { > + rc = -errno; > + goto out; > + } > + } else if (rc == 1) > + pd = region_get_pd_type(buf); > + > + if (region->persistence_domain < pd) > + region->persistence_domain = pd; > + } while (rc != EOF); > + > + rc = 0; > + > +out: > + fclose(pf); > + free(pd_path); > + return rc; > +} > + > static void *add_region(void *parent, int id, const char *region_base) > { > char buf[SYSFS_ATTR_SIZE]; > @@ -1762,6 +1831,7 @@ static void *add_region(void *parent, int id, const char *region_base) > struct ndctl_bus *bus = parent; > struct ndctl_ctx *ctx = bus->ctx; > char *path = calloc(1, strlen(region_base) + 100); > + int rc; > > if (!path) > return NULL; > @@ -1831,6 +1901,17 @@ static void *add_region(void *parent, int id, const char *region_base) > list_add(&bus->regions, ®ion->list); > > free(path); > + > + /* get the persistence domain attribs */ > + rc = region_persistence_scan(region); > + if (rc < 0) > + err(ctx, "%s: region persistence scan failed\n", > + ndctl_region_get_devname(region)); Nit: no need for 'rc' here, just follow the lead of the rest of the if() statements in this function and do if (region_persistence_scan(region) < 0) { err(... }
On Mon, Mar 19, 2018 at 4:45 PM, Dave Jiang <dave.jiang@intel.com> wrote: > Adding helper functions to iterate through sysfs region persistence domain > attribute. The region will display the domain with the most persistence for the > region. The bus will display the domain attribute with the least persistence > amongst all the regions. ndctl_bus_get_persistence_domain() and > ndctl_region_get_persistence_domain are exported. ndctl list will also display > the region persistence domain as well. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v2: > - Simplied scanning of persistence domain from Ross's comments. > > ndctl/lib/libndctl.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++ > ndctl/lib/libndctl.sym | 2 + > ndctl/lib/private.h | 1 + > ndctl/libndctl.h | 10 ++++++ > ndctl/list.c | 16 +++++++++ > 5 files changed, 116 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index a165e697..8f4e1745 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -180,6 +180,7 @@ struct ndctl_region { > } iset; > FILE *badblocks; > struct badblock bb; > + enum ndctl_persistence persistence_domain; > }; > > /** > @@ -755,6 +756,7 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > list_head_init(&bus->regions); > bus->ctx = ctx; > bus->id = id; > + bus->persistence_domain = PERSISTENCE_UNKNOWN; > > sprintf(path, "%s/dev", ctl_base); > if (sysfs_read_attr(ctx, path, buf) < 0 > @@ -916,6 +918,17 @@ NDCTL_EXPORT struct ndctl_bus *ndctl_bus_get_by_provider(struct ndctl_ctx *ctx, > return NULL; > } > > +NDCTL_EXPORT unsigned int > +ndctl_bus_get_persistence_domain(struct ndctl_bus *bus) > +{ > + struct ndctl_region *region; > + > + /* iterate through region to get the region persistence domain */ > + ndctl_region_foreach(bus, region) {} > + > + return bus->persistence_domain; There's no need to add this property to the 'struct ndctl_bus' object. Just do the aggregation live in the ndctl_region_foreach() loop that you already have.
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index a165e697..8f4e1745 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -180,6 +180,7 @@ struct ndctl_region { } iset; FILE *badblocks; struct badblock bb; + enum ndctl_persistence persistence_domain; }; /** @@ -755,6 +756,7 @@ static void *add_bus(void *parent, int id, const char *ctl_base) list_head_init(&bus->regions); bus->ctx = ctx; bus->id = id; + bus->persistence_domain = PERSISTENCE_UNKNOWN; sprintf(path, "%s/dev", ctl_base); if (sysfs_read_attr(ctx, path, buf) < 0 @@ -916,6 +918,17 @@ NDCTL_EXPORT struct ndctl_bus *ndctl_bus_get_by_provider(struct ndctl_ctx *ctx, return NULL; } +NDCTL_EXPORT unsigned int +ndctl_bus_get_persistence_domain(struct ndctl_bus *bus) +{ + struct ndctl_region *region; + + /* iterate through region to get the region persistence domain */ + ndctl_region_foreach(bus, region) {} + + return bus->persistence_domain; +} + NDCTL_EXPORT struct ndctl_btt *ndctl_region_get_btt_seed(struct ndctl_region *region) { struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); @@ -1755,6 +1768,62 @@ static int region_set_type(struct ndctl_region *region, char *path) return 0; } +static enum ndctl_persistence region_get_pd_type(char *name) +{ + if (strncmp("cpu_cache", name, 9) == 0) + return PERSISTENCE_CPU_CACHE; + else if (strncmp("memory_controller", name, 17) == 0) + return PERSISTENCE_MEM_CTRL; + else + return PERSISTENCE_UNKNOWN; +} + +static int region_persistence_scan(struct ndctl_region *region) +{ + struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); + char *pd_path; + FILE *pf; + char buf[64]; + int rc = 0; + enum ndctl_persistence pd = PERSISTENCE_NONE; + + if (asprintf(&pd_path, "%s/persistence_domain", + region->region_path) < 0) { + rc = -errno; + err(ctx, "region persist domain path allocation failure\n"); + return rc; + } + + pf = fopen(pd_path, "re"); + if (!pf) { + rc = -errno; + free(pd_path); + return rc; + } + + region->persistence_domain = PERSISTENCE_NONE; + do { + rc = fscanf(pf, "%s", buf); + if (rc == EOF) { + if (ferror(pf)) { + rc = -errno; + goto out; + } + } else if (rc == 1) + pd = region_get_pd_type(buf); + + if (region->persistence_domain < pd) + region->persistence_domain = pd; + } while (rc != EOF); + + rc = 0; + +out: + fclose(pf); + free(pd_path); + return rc; +} + static void *add_region(void *parent, int id, const char *region_base) { char buf[SYSFS_ATTR_SIZE]; @@ -1762,6 +1831,7 @@ static void *add_region(void *parent, int id, const char *region_base) struct ndctl_bus *bus = parent; struct ndctl_ctx *ctx = bus->ctx; char *path = calloc(1, strlen(region_base) + 100); + int rc; if (!path) return NULL; @@ -1831,6 +1901,17 @@ static void *add_region(void *parent, int id, const char *region_base) list_add(&bus->regions, ®ion->list); free(path); + + /* get the persistence domain attribs */ + rc = region_persistence_scan(region); + if (rc < 0) + err(ctx, "%s: region persistence scan failed\n", + ndctl_region_get_devname(region)); + + /* we are looking for the least persistence domain */ + if (region->bus->persistence_domain > region->persistence_domain) + region->bus->persistence_domain = region->persistence_domain; + return region; err_read: @@ -2093,6 +2174,12 @@ NDCTL_EXPORT struct badblock *ndctl_region_get_first_badblock(struct ndctl_regio return ndctl_region_get_next_badblock(region); } +NDCTL_EXPORT unsigned int +ndctl_region_get_persistence_domain(struct ndctl_region *region) +{ + return region->persistence_domain; +} + static struct nd_cmd_vendor_tail *to_vendor_tail(struct ndctl_cmd *cmd) { struct nd_cmd_vendor_tail *tail = (struct nd_cmd_vendor_tail *) diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 21276614..3209aefe 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -350,4 +350,6 @@ global: ndctl_dimm_cmd_new_ack_shutdown_count; ndctl_region_get_numa_node; ndctl_dimm_fw_update_supported; + ndctl_region_get_persistence_domain; + ndctl_bus_get_persistence_domain; } LIBNDCTL_14; diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h index 1cad06b7..e7787ce8 100644 --- a/ndctl/lib/private.h +++ b/ndctl/lib/private.h @@ -171,6 +171,7 @@ struct ndctl_bus { char *scrub_path; unsigned long cmd_mask; unsigned long nfit_dsm_mask; + enum ndctl_persistence persistence_domain; }; /** diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index f3a27411..151d48f0 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -115,6 +115,7 @@ int ndctl_bus_is_cmd_supported(struct ndctl_bus *bus, int cmd); unsigned int ndctl_bus_get_revision(struct ndctl_bus *bus); unsigned int ndctl_bus_get_id(struct ndctl_bus *bus); const char *ndctl_bus_get_provider(struct ndctl_bus *bus); +unsigned int ndctl_bus_get_persistence_domain(struct ndctl_bus *bus); int ndctl_bus_wait_probe(struct ndctl_bus *bus); int ndctl_bus_wait_for_scrub_completion(struct ndctl_bus *bus); unsigned int ndctl_bus_get_scrub_count(struct ndctl_bus *bus); @@ -305,6 +306,14 @@ struct badblock { unsigned long long offset; unsigned int len; }; + +enum ndctl_persistence { + PERSISTENCE_NONE = 0, + PERSISTENCE_MEM_CTRL, + PERSISTENCE_CPU_CACHE, + PERSISTENCE_UNKNOWN, +}; + struct ndctl_region; struct ndctl_region *ndctl_region_get_first(struct ndctl_bus *bus); struct ndctl_region *ndctl_region_get_next(struct ndctl_region *region); @@ -347,6 +356,7 @@ struct ndctl_region *ndctl_bus_get_region_by_physical_address(struct ndctl_bus * for (dimm = ndctl_region_get_first_dimm(region); \ dimm != NULL; \ dimm = ndctl_region_get_next_dimm(region, dimm)) +unsigned int ndctl_region_get_persistence_domain(struct ndctl_region *region); int ndctl_region_is_enabled(struct ndctl_region *region); int ndctl_region_enable(struct ndctl_region *region); int ndctl_region_disable_invalidate(struct ndctl_region *region); diff --git a/ndctl/list.c b/ndctl/list.c index 0ca5b6de..f3701ea9 100644 --- a/ndctl/list.c +++ b/ndctl/list.c @@ -73,6 +73,7 @@ static struct json_object *region_to_json(struct ndctl_region *region, struct ndctl_interleave_set *iset; struct ndctl_mapping *mapping; unsigned int bb_count = 0; + enum ndctl_persistence pd; int numa; if (!jregion) @@ -174,6 +175,21 @@ static struct json_object *region_to_json(struct ndctl_region *region, if ((flags & UTIL_JSON_MEDIA_ERRORS) && jbbs) json_object_object_add(jregion, "badblocks", jbbs); + pd = ndctl_region_get_persistence_domain(region); + switch (pd) { + case PERSISTENCE_CPU_CACHE: + jobj = json_object_new_string("cpu_cache"); + break; + case PERSISTENCE_MEM_CTRL: + jobj = json_object_new_string("memory_controller"); + break; + default: + jobj = NULL; + } + + if (jobj) + json_object_object_add(jregion, "persistence_domain", jobj); + return jregion; err: fail("\n");
Adding helper functions to iterate through sysfs region persistence domain attribute. The region will display the domain with the most persistence for the region. The bus will display the domain attribute with the least persistence amongst all the regions. ndctl_bus_get_persistence_domain() and ndctl_region_get_persistence_domain are exported. ndctl list will also display the region persistence domain as well. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v2: - Simplied scanning of persistence domain from Ross's comments. ndctl/lib/libndctl.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++ ndctl/lib/libndctl.sym | 2 + ndctl/lib/private.h | 1 + ndctl/libndctl.h | 10 ++++++ ndctl/list.c | 16 +++++++++ 5 files changed, 116 insertions(+)