> -----Original Message----- > From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] > Sent: Wednesday, February 04, 2015 12:32 PM > To: Savolainen, Petri (NSN - FI/Espoo) > Cc: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: > odp_cpumask_to_str() return chars written or error > > On 4 February 2015 at 11:13, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolai...@nsn.com> wrote: > > > > > >> -----Original Message----- > >> From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] > >> Sent: Wednesday, February 04, 2015 11:50 AM > >> To: Savolainen, Petri (NSN - FI/Espoo) > >> Cc: lng-odp@lists.linaro.org > >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: > >> odp_cpumask_to_str() return chars written or error > >> > >> On 4 February 2015 at 10:27, Savolainen, Petri (NSN - FI/Espoo) > >> <petri.savolai...@nsn.com> wrote: > >> > > >> > > >> >> -----Original Message----- > >> >> From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] > >> >> Sent: Wednesday, February 04, 2015 11:20 AM > >> >> To: Savolainen, Petri (NSN - FI/Espoo) > >> >> Cc: lng-odp@lists.linaro.org > >> >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: > >> >> odp_cpumask_to_str() return chars written or error > >> >> > >> >> On 4 February 2015 at 09:38, Savolainen, Petri (NSN - FI/Espoo) > >> >> <petri.savolai...@nsn.com> wrote: > >> >> > > >> >> > > >> >> >> -----Original Message----- > >> >> >> From: lng-odp-boun...@lists.linaro.org [mailto:lng-odp- > >> >> >> boun...@lists.linaro.org] On Behalf Of ext Savolainen, Petri (NSN > - > >> >> >> FI/Espoo) > >> >> >> Sent: Wednesday, February 04, 2015 10:31 AM > >> >> >> To: ext Ola Liljedahl > >> >> >> Cc: lng-odp@lists.linaro.org > >> >> >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: > >> >> >> odp_cpumask_to_str() return chars written or error > >> >> >> > >> >> >> > >> diff --git a/include/odp/api/cpumask.h > >> b/include/odp/api/cpumask.h > >> >> >> > >> index 7482899..65969c3 100644 > >> >> >> > >> --- a/include/odp/api/cpumask.h > >> >> >> > >> +++ b/include/odp/api/cpumask.h > >> >> >> > >> @@ -18,12 +18,20 @@ > >> >> >> > >> extern "C" { > >> >> >> > >> #endif > >> >> >> > >> > >> >> >> > >> +#include <sys/types.h> > >> >> >> > >> +#include <odp/config.h> > >> >> >> > >> + > >> >> >> > >> /** @addtogroup odp_scheduler > >> >> >> > >> * CPU mask operations. > >> >> >> > >> * @{ > >> >> >> > >> */ > >> >> >> > >> > >> >> >> > >> - /** > >> >> >> > >> +/** > >> >> >> > >> + * @def ODP_CPUMASK_BUFSIZE > >> >> >> > >> + * Minimum size of output buffer for odp_cpumask_to_str() > >> >> >> > >> + */ > >> >> >> > > > >> >> >> > > ODP_CPUMASK_STRLEN is better. Better to use term string when > >> it's a > >> >> >> > character string. > >> >> But is it not a character string. > >> >> > >> >> The user specifies a buffer and a buffer size. The function writes a > >> >> string into that buffer (if it fits into the buffer). > >> >> The buffer then *contains* a string (which will be shorter that the > >> >> buffer size value specified, e.g. because trailing CPU bits may not > be > >> >> set and also the string length does not include the terminating null > >> >> char). But a string and a buffer are not the same thing. > >> >> > >> >> >> > >> >> >> This one is missing from v5. > >> >> >> > >> >> >> Better name it STRLEN when it used for _to_str() and _from_str() > >> >> function > >> >> >> calls. > >> >> I think this is a poor choice of name. The length of a string is the > >> >> number of characters in the string (excluding the terminating null > >> >> char). But we need to pass the buffer size. > >> >> > >> >> >> > >> >> >> char mask_str[ODP_CPUMASK_STRLEN]; > >> >> >> memset(mask_str, 0, sizeof(mask_str)); > >> >> Why is the memset() here? It is not needed. > >> >> > >> >> >> odp_cpumask_to_str(mask, mask_str, sizeof(mask_str)); > >> >> >> odp_cpumask_from_str(mask, mask_str); > >> >> Yes this is how it is done, before my patch and after my patch. > >> >> > >> >> >> > >> >> >> > >> >> >> -Petri > >> >> > > >> >> > > >> >> > Or ODP_CPUMASK_STRLEN_MAX or ODP_CPUMASK_STR_MAX_CHARS ... > >> >> > > >> >> > The point is that, it's defining the max number of chars (not > bytes) > >> >> needed for the string output - and it cannot be mixed with the > >> >> odp_cpumask_t (buffer) size. > >> >> odp_cpumask_t has a separate well-defined type (no char bufs or void > >> >> pointers used when you are passing an odp_cpumask_t value), I see no > >> >> risk for mixing it with ODP_CPUMASK_BUFSIZE. > >> > > >> > > >> > If you reserve ODP_CPUMASK_BUFSIZE for the string API. What if we add > >> odp_cpumask_to_u64(), odp_cpumask_to_u8(),odp_cpumask_to_buf(), etc API > >> calls later on? It's better to reserve BUF for those buffers and use > STR > >> for strings (no matter if sting initialized or not before the call, > output > >> is still a string). > >> Purely hypothetical. One can invent any number of counter arguments, > >> that does not make them realistic or meaningful. > >> What if we add other odp_cpumask string calls that need some (other) > >> type of buffer to hold their strings? > >> > >> But I do want to make the name as easy as possible to understand and > >> try to see your criticism in a constructive light. > >> Perhaps ODP_CPUMASK_STR_BUFSIZE? Buffer size for cpumask strings. > >> > >> The buffer size is still different from the string length just as a > >> buffer is not a string. > >> > > > > And "hypothetical" seems to be favorite counter-counter argument... > Because you are the expert at finding hypothetical counter arguments > as soon as anyone else has a different opinion. This happens like ten > times every time we have a call. > > > We already had _to_u64() but removed it from the API. It may have a > comeback (in some form), if we need to start packing these masks into > messages and send those between devices (e.g. transmit portable cpumask > definitions between control and data plane devices). > What's wrong with these cpumask strings for that purpose? Variable > length yes, I think that is a strength. You don't want the control > plane and the protocols to have dependencies on how many CPU's there > are in the dataplane. ASCII encoded instead of binary (so taking > ~twice the size), is that a problem?
The first problem is not leaving door open for binary formats. The second problem is performance. Strings are typically for debugging, logging, etc (low call rate). The function creating the string may be tempted to use C library string/stdio calls and not optimize for performance. Binary format could have clear performance target (for messaging, not for debugging). -Petri > > If you start by specifying the problem, more people can collaborate on > finding the best solution. > > > > > ODP_CPUMASK_STR_SIZE would be shorter. > I can think of even shorter names. It doesn't mean shorter names are > better. At least you are using SIZE and not LEN. I think we are > converging. > > > > > ODP_CPUMASK_STR_XXX, with or without BUF - is understandable to me. > > > > > > -Petri > > > > _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp
