[ovs-dev] [dpdk patch 1/8] ovs-numa: Add ovs-numa.{c, h} for extracting and storing cpu socket and cpu core info.

[Alex Wang]

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?