Date:	Thu, 23 Aug 2012 18:35:20 +0000
From:	"Vick, Matthew" <matthew.vick@...el.com>
To:	Richard Cochran <richardcochran@...il.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 11/13] igb: Update PTP function names/variables and
 locations.

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@...il.com]
> Sent: Thursday, August 23, 2012 10:54 AM
> To: Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@...emloft.net; netdev@...r.kernel.org;
> gospo@...hat.com; sassmann@...hat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> 
> > > >  #ifdef CONFIG_IGB_PTP
> > > > -static int igb_ethtool_get_ts_info(struct net_device *dev,
> > > > +static int igb_get_ts_info(struct net_device *dev,
> > >
> > > I like the old name better.
> >
> > The old name is out of the coding style of igb. Every other function
> is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> igb_ethtool_complete. Are you suggesting we add _ethtool to every
> ethtool function in igb?
> 
> No, just leave the names alone, and keep the functions where they are.
> It is just churn.
> 
> One of the most useful ways to understand code (at least for me) is to
> use git blame. It tells you when code was added, what the reason was,
> and how the change looks in context. By moving and renaming willy
> nilly, you are obscuring this valuable information.

Repairing is not churn. I think it's better to make the code clearer than require developers use git blame to figure out who named a function in a silly way. I agree, it kind of stinks to lose that history, but I'd rather not skip making the right change because of git blame concerns.

> 
> [...]
> 
> >
> > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > global.
> 
> So how does calling two global functions in series improve performance?

It isn't calling two global functions. It's calling a global function that calls a static function.

> > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > even be called in clean_tx_irq, FWIW.
> 
> If this is part of some larger plan, then it would help to see that
> plan.

Definitely fair enough. I will work with Jeff to try and push the whole series next time (at the very least, I'd like to re-spin the series for the ethtool query patch).

> > > >  /**
> > > >   * igb_clean_tx_irq - Reclaim resources after transmit completes
> > > >   * @q_vector: pointer to q_vector containing needed info @@
> > > > -5827,7
> > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector
> > > *q_vector)
> > > >
> > > >  #ifdef CONFIG_IGB_PTP
> > > >  		/* retrieve hardware timestamp */
> > > > -		igb_tx_hwtstamp(q_vector, tx_buffer);
> > > > +		igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
> > >
> > > This name stinks, too. You know that you can have time stamping all
> > > by itself, right? It is logically separate from the ptp clock
> stuff.
> > >
> > > This patch doesn't really improve the driver at all, IMHO.
> > >
> > > Thanks,
> > > Richard
> >
> > Yes, I'm aware. But, as it stands today, we don't use it for anything
> else. If the function is feature specific, then we should be calling it
> out as such.
> 
> Right now the time stamping is being equated with the clock functions,
> but it really should be decoupled. The 82580 can time stamp every
> received packet, which can be interesting for performance monitoring,
> even without PTP (and adding *that* would be a useful change).

As Jake said, there's no support for hardware timestamping without PTP today. As such, if the function is only useful for that feature, then I think it's less ambiguous to leave it the way I've coded it today. If we de-couple hardware timestamping and clock synchronization, which I wouldn't be opposed to, then the function name can reflect becoming more generic. As it stands, I'd rather not call something generic when it isn't. I think it's misleading, personally.

> > I'm sorry you feel like this patch doesn't improve the driver. The
> goal is code cleanup and consistency, both of which I consider to be
> driver improvements and is why I made the patches.
> 
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as
> well be making *that* driver consistent with this one.
> 
> Thanks,
> Richard

Actually, yes, the code *was* dirty, because it breaks the coding style of the rest of the driver and isn't as self-documenting as it could be, and the inline-able functions you claim I'm destroying called global functions anyway. Also, just because igb came first doesn't mean we should ignore porting better or clearer solutions from later drivers. If something is better or clearer, I think we should use be using it.

Cheers,
Matthew
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists