On Thu, Apr 16, 2009 at 4:07 PM, Peter Tribble <peter.trib...@gmail.com> wrote:
> On Tue, Apr 7, 2009 at 8:25 PM, Jason King <ja...@ansipunx.net> wrote:
>> To get around the hurdle of all kstats being effectively private,
>> which in turn makes it difficult to effectively build any sort of
>> tools that use them,
>
> That hasn't stopped plenty of tools being developed, though.
>
>> I'm proposing adding stability attributes to
>> kstats, similar to how dtrace providers have stability attributes. (I
>> believe this was originally suggested by Dan Price, though I'd have to
>> look -- the point is I don't want to falsely take credit for the
>> idea). The immediate need is as a prerequisite for the SNMP
>> performance MIB, so that it can be done right.
>>
>> I'd like to throw out a proposed design for discussion (see below).
>> If a consensus can be reached, then I'd like to create an ARC case for
>> submission. I'm not sure it's big enough to warrant it's own project
>> (though if others disagree, I'm fine with that as well).
>>
>> Stability attributes:
>>
>> The thinking is unless there's a good reason to diverge, for
>> consistency (and sanity of someone trying to figure out what to use),
>> I think keeping it analogous to the dtrace attributes makes the most
>> sense:
>>
>> typedef uint8_t kstat_stability_t;
>
> Why a new type?
>
>> KSTAT_STABILITY_INTERNAL internal to kstat system
>> KSTAT_STABILITY_PRIVATE private to ON, the default for all kstats
>> KSTAT_STABILITY_OBSOLETE scheduled for removal
>> KSTAT_STABILITY_EXTERNAL maintained by a 3rd party
>> KSTAT_STABILITY_UNSTABLE new or rapidly changing
>> KSTAT_STABILITY_EVOLVING less rapidly changing
>> KSTAT_STABILITY_STABLE mature interface
>> KSTAT_STABILITY_STANDARD industry standard
>> KSTAT_STABILITY_MAX maximum valid stability
>
> That's a lot. About 7 or 8 too many.
>
> At most it's a binary choice: these are safe to use, or they aren't.
> I'm not sure I
> see utility in finer shades of grey.
>
> The classification is really two-dimensional: is the name/existence of the
> kstat
> stable; and then whether the data it exports can be relied on.
Based on current ARC definitions, here's the updated proposal:
typedef enum {
KSTAT_STABILITY_PRIVATE = 0, /* not intended for general use */
KSTAT_STABILITY_VOLATILE, /* may change in a patch or micro
release */
KSTAT_STABILITY_UNCOMITTED, /* may change in a minor release */
KSTAT_STABILITY_COMITTED, /* may change in a major release */
} kstat_stability_t;
(this is probably better for debugging vs. #defines)
We could have the notion of name stability & data stability and just
extend things. I also thought of an alternate api. With this either:
int kstat_set_stability(kstat_t *ks, kstat_stability_t name_s,
kstat_stability_t data_s);
or
kstat_t *kstat_create_ex(const char *ks_module, int ks_instance,
const char *ks_name, const char *ks_class, uchar_t ks_type,
ulong_t ks_ndata, uchar_t ks_flag, kstat_stability_t name_s,
kstat_stability_t data_s, int res);
I'll go along with setting per kstat_t. If someone wants to set
different instances to different values, it might be strange but not
prohibited.
The idea with kstat_create_ex is twofold:
1. Stability information is set at creation time, no need for an extra
step. if kstat_create is called instead, it is equivalent to calling
kstat_create_ex with name_s and data_s set to KSTAT_STABILITY_PRIVATE.
2. the res flag can be set to 0 for now, but should allow for future
extension if needed.
This could still fit in the kstat_t reserved field.
>
> (And, as someone who writes kstat consumers, I would simply ignore the
> stability levels anyway. I care far more about the data that's contained in
> the
> kstats.)
>
>> Kernel Interfaces:
>>
>> int kstat_set_stability(const char *ks_module, const char *ks_name,
>> kstat_stability_t stability);
>> Desc: set the kstat to a given stability level
>> Returns: 0 - success
>> EINVAL - stability level isn't valid
>> ENOENT - kstat doesn't exist
>
> Why a separate call? Isn't it possible to extend the bit-field of possible
> flags?
>
> (That assumes that the stability is described as KSTAT_FLAG_STABLE,
> which is either set or not. Doing multiple stability levels this way
> gets a little
> harder...)
>
>> ??: should this be mutable, or should it be allowed to only be set
>> once, and return an error from there on out?
>
> If you were going to have a separate call, then having it between kstat_create
> and kstat_install would be necessary. In other words, kstat_set_stability
> would
> fail unless KSTAT_FLAG_INVALID were set.
>
>> int kstat_get_stability(const char *ks_module, const char *ks_name,
>> kstat_stability_t *stability);
>> Returns: 0 - stability value in stability. If the stability value was
>> not explicitly set, *stability contains KSTAT_STABILITY_PRIVATE.
>> ENOENT - kstat does not exist.
>>
>> User interfaces:
>>
>> int kstat_get_stability(kstat_ctl_t *kc, const char *ks_module, const
>> char *ks_name, kstat_stability_t *stability)
>> Similar to kernel interface
>> Returns 0 on success, -1 on failure. On failure, errno is set to:
>> ENOENT - kstat does not exist.
>
> Yuk. Why a separate call? Makes life far more complicated, as you have to
> catch kstats going, and really you need to check that the kstat you're talking
> to is the same one you thought you were talking to. In fact, rather than using
> module/name (you need instance as well) it might be easier to simply use
> the kstat or its KID as the argument to identify a kstat uniquely.
>
> Another possible design would be to do away with the separate call,
> and simply appropriate the (currently unused)ks_resv field in the kstat
> structure to hold the stability data.
>
>> ??: How should the perl module be updated.
>
> Well, I would just have (optionally, so the output format doesn't
> change by default) the stability level printed out just like the class
> is. That's what I would do for JKstat, anyway.
>
> --
> -Peter Tribble
> http://www.petertribble.co.uk/ - http://ptribble.blogspot.com/
>
_______________________________________________
perf-discuss mailing list
perf-discuss@opensolaris.org
