Thanks Ben, for the review,
> > +
> > +/* To avoid sparse warning. */
> > +#ifdef __linux__
> ...
> > +#endif /* __linux__ */
>
> I didn't see the connection to sparse at first. I'm still not sure
> there is one. Maybe a better comment:
> /* On non-Linux, these functions are defined inline in ovs-numa.h. */
>
>
Yeah, this is better. It is cumbersome to describe that autoconf defines
the LINUX that cause the compilation of ovs-numa.c, but sparse does
not __linux__.
> + /* Constructs the path to node /sys/devices/system/nodeX. */
> + path = xasprintf("/sys/devices/system/node/node%d", i);
> +
> + if (strlen(path) >= PATH_MAX) {
> + VLOG_WARN("Path to cpu socket %d exceeds the length limit",
> i);
> + break;
> + }
> The above check seems odd. PATH_MAX is about 4 kB, I think. I can't
> see any possible way that path would exceed it and, even if it did,
> why not let opendir() catch the problem?
>
>
Makes sense, I'll remove it,.
> In discover_sockets_and_cores(), you can remove endptr. It is
> assigned a value that is never used.
>
Thx, removed it,
>
> It looks to me like ovs-numa does not verify that every socket has at
> least one core. Should it?
>
I think it is okay to not check for that, instead, the code will prevent the
creation of pmd threads on that cpu socket and issue warning...
also, I added this, to log the number cores on each socket:
@@ -113,11 +107,18 @@ discover_sockets_and_cores(void)
n_cpus++;
}
}
- free(subdir);
+ VLOG_INFO("Discovered %"PRIu64" CPU cores on CPU socket %d",
+ list_size(&s->cores), i);
+ free(path);
+ closedir(dir);
} else {
+ if (errno != ENOENT) {
+ VLOG_WARN("opendir(%s) failed (%s)", path,
+ ovs_strerror(errno));
+ }
+ free(path);
break;
}
do you think it is good?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
