Re: [PATCH 1/3] Adding a some new probes to the networking.stp tapset

[David Smith <dsmith@redhat.com>]

On 09/16/2009 01:37 PM, leitao@linux.vnet.ibm.com wrote:
> A tapset that helps those who are working with network devices.
> These new fnctions try to cover almost all functions related to
> these network devices.

In general, these look pretty good, but...

> +/**
> + * probe netdev.change_mac - Called when the netdev_name has the MAC changed
> + * @dev_name: The device that will have the MTU changed
> + * @mac_len: The MAC length
> + * @old_mac: The current MAC address
> + * @new_mac: The new MAC address
> + */
> +probe netdev.change_mac
> +	= kernel.function("dev_set_mac_address")
> +{
> +	dev_name = get_netdev_name($dev)	
> +	mac_len = $dev->addr_len
> +
> +	// Old MAC Address
> +	zero = $dev->dev_addr[0]
> +	one =  $dev->dev_addr[1]
> +	two =  $dev->dev_addr[2]
> +	three =$dev->dev_addr[3] 
> +	four = $dev->dev_addr[4]
> +	five = $dev->dev_addr[5]
> +	old_mac = sprintf("%02x:%02x:%02x:%02x:%02x:%02x",
> +			 zero, one, two, three, four, five)
> +
> +	// New MAC Address
> +	zero = $sa->sa_data[0]
> +	one  = $sa->sa_data[1]
> +	two  = $sa->sa_data[2]
> +	three =$sa->sa_data[3] 
> +	four  =$sa->sa_data[4] 
> +	five = $sa->sa_data[5]
> +	new_mac = sprintf("%02x:%02x:%02x:%02x:%02x:%02x",
> +			 zero, one, two, three, four, five)
> +}

Because of the way the optimizer works, reusing those temporary
variables probably isn't a good idea.  If you call that function and
only use 'new_mac', here's the pass 2 output:

====
kernel.function("dev_set_mac_address@net/core/dev.c:3775") /*
pc=_stext+0x308ab1 */ /* <- netdev.change_mac =
kernel.function("dev_set_mac_address") <- netdev.change_mac */
  # locals
  zero:long
  one:long
  two:long
  three:long
  four:long
  five:long
  new_mac:string
{
(zero) = (_dwarf_tvar_get_dev_2())
(one) = (_dwarf_tvar_get_dev_3())
(two) = (_dwarf_tvar_get_dev_4())
(three) = (_dwarf_tvar_get_dev_5())
(four) = (_dwarf_tvar_get_dev_6())
(five) = (_dwarf_tvar_get_dev_7())
(zero) = (_dwarf_tvar_get_sa_8())
(one) = (_dwarf_tvar_get_sa_9())
(two) = (_dwarf_tvar_get_sa_10())
(three) = (_dwarf_tvar_get_sa_11())
(four) = (_dwarf_tvar_get_sa_12())
(five) = (_dwarf_tvar_get_sa_13())
(new_mac) = (sprintf("%02x:%02x:%02x:%02x:%02x:%02x", zero, one, two,
three, four, five))
printf("%s", new_mac)
}
====

The optimizer removed the assignment to old_mac, but it couldn't
optimize the 1st six assignments and their _dwarf_tvar_get_dev_*
function calls away.

So, I think it would be better to trade off the six assignments and the
call to the _dwarf_tvar_get_dev_* and make separate sets of temporary
variables.  You'll end up increasing the number of temporaries that way,
but it should execute faster if only one of the mac addresses is used.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)