Re: [PATCH 2/2 V3] Allow named mask to be used as default mask

[Maynard J. <may...@us...>]

On 06/12/2013 02:37 AM, su...@gm... wrote:
> From: Suravee Suthikulpanit <sur...@am...>
> 
> Current libop does not allow named mask to be used as default mask.
> This causes the tools to fail when user does not specify the named
> mask of an event and relying on the numerical default unit mask
> which could have duplication.
> 
> Signed-off-by: Suravee Suthikulpanit <sur...@am...>
> ---
> Changes in V3:
> 	- Fix ophelp to properly print out the mask name and default mask name

Thanks for this update to your patch set, Suravee.  With this patch, the ophelp for the ld_blocks event on my Sandybridge looks like this:

==================================
ld_blocks: (counter: all)
        blocked loads (min count: 100000)
        Unit masks (default 0x1)
        ----------
        0x01: blocked loads due to store buffer blocks with unknown data. (name:data_unknown)
              (extra:)
        0x02: loads blocked by overlapping with store buffer that cannot be forwarded (name:
              store_forward) (extra:)
        0x08: This event counts the number of times that split load operations are temporarily
              blocked because all resources for handling the split accesses are in use. (name:
              no_sr) (extra:)
        0x10: Number of cases where any load is blocked but has no DCU miss. (name:all_block)
              (extra:)
==================================

A few comments about this change:
1) I would prefer the name field to be highlighted better -- i.e., by placing in front of the description.
2) This is probably more a question for *Andi*, but now that we're including a name field, is there any reason to show the "extra:" stuff?  Some "extra:" fields are empty, while others have things like "edge cmask=1".  But is that information useful to the person running oprofile?  I defer to your judgment on that.
3) The operf man page says the following about unit masks:

              When specifying a unit mask value, it may be either a numerical value (hex values
              must begin with "0x") or a string (i.e, symbolic name) which  matches  the  first
              word  in  the unit mask description. Specifying a symbolic name for the unit mask
              is valid only for unit masks having "extra:" parameters, as shown by  the  output
              of  ophelp.  Unit masks with "extra:" parameters must be specified using the sym-
              bolic name, thus it is not possible to include them in  bitmask  values.   If  no
              unit mask is specified, the default unit mask value for the event is used.

  Other man pages, as well as our user manual, document "named" unit masks similarly.  So we'll need a patch to update the user manual and man pages.  I recently updated those docs and know where the changes need to be made, so I'd be happy to do that.  We just need to come to agreement on exactly how the ophelp output will be formatted.

Thanks!
-Maynard
> 
>  libop/op_events.c |   86 ++++++++++++++++++++++++++++++++++++++++-------------
>  libop/op_events.h |    2 ++
>  utils/ophelp.c    |   16 ++++++++--
>  3 files changed, 82 insertions(+), 22 deletions(-)
> 
> diff --git a/libop/op_events.c b/libop/op_events.c
> index 276386d..bba7b43 100644
> --- a/libop/op_events.c
> +++ b/libop/op_events.c
> @@ -197,7 +197,12 @@ static void parse_um(struct op_unit_mask * um, char const * line)
>  			if (seen_default)
>  				parse_error("duplicate default: tag");
>  			seen_default = 1;
> -			um->default_mask = parse_hex(tagend);
> +			if (0 != strncmp(tagend, "0x", 2)) {
> +				um->default_mask_name = op_xstrndup(
> +					tagend, valueend - tagend);
> +			} else {
> +				um->default_mask = parse_hex(tagend);
> +			}
>  		} else {
>  			parse_error("invalid unit mask tag");
>  		}
> @@ -215,32 +220,50 @@ static void parse_um(struct op_unit_mask * um, char const * line)
> 
> 
>  /* \t0x08 (M)odified cache state */
> -/* \t0x08 extra:inv,cmask=... (M)odified cache state */
> +/* \t0x08 extra:inv,cmask=... mod_cach_state (M)odified cache state */
>  static void parse_um_entry(struct op_described_um * entry, char const * line)
>  {
>  	char const * c = line;
> 
> +	/* value */
>  	c = skip_ws(c);
>  	entry->value = parse_hex(c);
> -	c = skip_nonws(c);
> 
> +	/* extra: */
> +	c = skip_nonws(c);
>  	c = skip_ws(c);
> +	if (!*c)
> +		goto invalid_out;
> +
>  	if (strisprefix(c, "extra:")) {
>  		c += 6;
>  		entry->extra = parse_extra(c);
> +		/* named mask */
>  		c = skip_nonws(c);
> -	} else
> -		entry->extra = 0;
> -
> -	if (!*c)
> -		parse_error("invalid unit mask entry");
> +		c = skip_ws(c);
> +		if (!*c)
> +			goto invalid_out;
> 
> -	c = skip_ws(c);
> +		/* "extra:" !!ALWAYS!! followed by named mask */
> +		entry->name = op_xstrndup(c, strcspn(c, " \t"));
> +		c = skip_nonws(c);
> +		c = skip_ws(c);
> +	} else {
> +		entry->extra = 0;
> +	}
> 
> -	if (!*c)
> -		parse_error("invalid unit mask entry");
> +	/* desc */
> +	if (!*c) {
> +		/* This is a corner case where the named unit mask entry
> +		 * only has one word.  This should really be fixed in the
> +		 * unit_mask file */
> +		entry->desc = xstrdup(entry->name);
> +	} else
> +		entry->desc = xstrdup(c);
> +	return;
> 
> -	entry->desc = xstrdup(c);
> +invalid_out:
> +	parse_error("invalid unit mask entry");
>  }
> 
> 
> @@ -605,10 +628,18 @@ static int check_unit_mask(struct op_unit_mask const * um,
>  				"(%s)\n", um->name, cpu_name);
>  			err = EXIT_FAILURE;
>  		}
> -	} else {
> -		for (i = 0; i < um->num; ++i) {
> -			if (um->default_mask == um->um[i].value)
> -				break;
> +	} else if (um->unit_type_mask == utm_exclusive) {
> +		if (um->default_mask_name) {
> +			for (i = 0; i < um->num; ++i) {
> +				if (0 == strcmp(um->default_mask_name,
> +						um->um[i].name))
> +					break;
> +			}
> +		} else {
> +			for (i = 0; i < um->num; ++i) {
> +				if (um->default_mask == um->um[i].value)
> +					break;
> +			}
>  		}
> 
>  		if (i == um->num) {
> @@ -708,6 +739,7 @@ static void load_events(op_cpu cpu_type)
>  	unit_mask->unit_type_mask = utm_mandatory;
>  	unit_mask->um[0].extra = 0;
>  	unit_mask->um[0].value = 0;
> +	unit_mask->um[0].name = xstrdup("");
>  	unit_mask->um[0].desc = xstrdup("No unit mask");
>  	unit_mask->used = 1;
> 
> @@ -1335,10 +1367,18 @@ static void extra_check(struct op_event *e, char *name, unsigned w)
>  	}
>  }
> 
> -static void do_resolve_unit_mask(struct op_event *e, struct parsed_event *pe, u32 *extra)
> +static void do_resolve_unit_mask(struct op_event *e,
> +	struct parsed_event *pe, u32 *extra)
>  {
>  	unsigned i;
> 
> +	/* If not specified um and the default um is name type
> +	 * we populate pe unitmask name with default name */
> +	if ((e->unit->default_mask_name != NULL) &&
> +		(pe->unit_mask_name == NULL)) {
> +		pe->unit_mask_name = xstrdup(e->unit->default_mask_name);
> +	}
> +
>  	for (;;) {
>  		if (pe->unit_mask_name == NULL) {
>  			/* For numerical unit mask */
> @@ -1374,9 +1414,15 @@ static void do_resolve_unit_mask(struct op_event *e, struct parsed_event *pe, u3
>  		} else {
>  			/* For named unit mask */
>  			for (i = 0; i < e->unit->num; i++) {
> -				int len = strcspn(e->unit->um[i].desc, " \t");
> -				if (!strncmp(pe->unit_mask_name, e->unit->um[i].desc,
> -					    len) && pe->unit_mask_name[len] == '\0')
> +				int len = 0;
> +
> +				if (e->unit->um[i].name)
> +					len = strlen(e->unit->um[i].name);
> +
> +				if (len
> +				&&  (!strncmp(pe->unit_mask_name,
> +					      e->unit->um[i].name, len))
> +				&&  (pe->unit_mask_name[len] == '\0'))
>  					break;
>  			}
>  			if (i == e->unit->num) {
> diff --git a/libop/op_events.h b/libop/op_events.h
> index a437f2a..33114f7 100644
> --- a/libop/op_events.h
> +++ b/libop/op_events.h
> @@ -56,9 +56,11 @@ struct op_unit_mask {
>  	u32 num;		/**< number of possible unit masks */
>  	enum unit_mask_type unit_type_mask;
>  	u32 default_mask;	/**< only the gui use it */
> +	char * default_mask_name;
>  	struct op_described_um {
>  	        u32 extra;
>  		u32 value;
> +		char * name;
>  		char * desc;
>  	} um[MAX_UNIT_MASK];
>  	struct list_head um_next; /**< next um in list */
> diff --git a/utils/ophelp.c b/utils/ophelp.c
> index 2f83528..a3376f0 100644
> --- a/utils/ophelp.c
> +++ b/utils/ophelp.c
> @@ -136,8 +136,13 @@ static void help_for_event(struct op_event * event)
> 
>  	if (strcmp(event->unit->name, "zero")) {
> 
> -		printf("\tUnit masks (default 0x%x)\n",
> -		       event->unit->default_mask);
> +		if (event->unit->default_mask_name) {
> +			printf("\tUnit masks (default %s)\n",
> +			       event->unit->default_mask_name);
> +		} else {
> +			printf("\tUnit masks (default 0x%x)\n",
> +			       event->unit->default_mask);
> +		}
>  		printf("\t----------\n");
> 
>  		for (j = 0; j < event->unit->num; j++) {
> @@ -148,6 +153,13 @@ static void help_for_event(struct op_event * event)
>  			if (event->unit->um[j].extra) {
>  				u32 extra = event->unit->um[j].extra;
> 
> +				if (event->unit->um[j].name) {
> +					word_wrap(14, &column, " (name:");
> +					word_wrap(14, &column,
> +						event->unit->um[j].name);
> +					word_wrap(14, &column, ")");
> +				}
> +
>  				word_wrap(14, &column, " (extra:");
>  				if (extra & EXTRA_EDGE)
>  					word_wrap(14, &column, " edge");
>