svn commit: r241616 - in head/sys: dev/ixgbe net

[John Baldwin]

On Tuesday, October 16, 2012 5:10:56 pm Maksim Yevmenkin wrote:
> On Tue, Oct 16, 2012 at 2:02 PM, John Baldwin <jhb at freebsd.org> wrote:
> > On Tuesday, October 16, 2012 4:18:16 pm Maksim Yevmenkin wrote:
> >> Author: emax
> >> Date: Tue Oct 16 20:18:15 2012
> >> New Revision: 241616
> >> URL: http://svn.freebsd.org/changeset/base/241616
> >>
> >> Log:
> >>   introduce concept of ifi_baudrate power factor. the idea is to work
> >>   around the problem where high speed interfaces (such as ixgbe(4))
> >>   are not able to report real ifi_baudrate. bascially, take a spare
> >>   byte from struct if_data and use it to store ifi_baudrate power
> >>   factor. in other words,
> >>
> >>   real ifi_baudrate = ifi_baudrate * 10 ^ ifi_baudrate power factor
> >>
> >>   this should be backwards compatible with old binaries. use ixgbe(4)
> >>   as an example on how drivers would set ifi_baudrate power factor
> >>
> >>   Discussed with:     kib, scottl, glebius
> >>   MFC after:  1 week
> >
> > It would be a lot nicer if you could still allow one to use more
> > readable things like IF_Gbps(10).  Note that we do have a 40G driver
> > (mlxen) as well.
> >
> > Maybe a helper 'if_set_baudrate(ifp, IF_Gbps(10))' that would DTRT.
> > (It could be a static inline or some such).  I would just like to
> > keep the readability.
> 
> well, yes, i thought about it, but decided not to do it right away. we
> could provide shortcuts/macros for "popular" baudrates, i.e. 1, 10, 40
> and 100 Gbps. while ixgbe(4) example is not ideal, i thought it still
> was pretty readable :)

I don't really find it all that readable.  IF_Gbps(1) looks like a typo
to a casual reader.  I really think you should have something like:

static __inline void
if_initbaudrate(struct ifnet *ifp, uintmax_t baud)
{

	ifp->if_baudrate_pf = 0;
	while (baud > ULONG_MAX) {
		baud /= 10;
		ifp->if_baudrate_pf++;
	}
	ifp->if_baudrate = baud;
}

I tested and with -O the compiler inlines that completely:

void
foo(void)
{
	if_initbaudrate(&i1g, IF_Gbps(1));
	if_initbaudrate(&i10g, IF_Gbps(10));
	if_initbaudrate(&i40g, IF_Gbps(40));
}

Disassembly (when ULONG_MAX is changed to UINT_MAX to simulate a 32-bit
platform):

0000000000000000 <foo>:
   0:   c6 05 00 00 00 00 00    movb   $0x0,0(%rip)        # 7 <foo+0x7>
   7:   48 c7 05 00 00 00 00    movq   $0x3b9aca00,0(%rip)        # 12 <foo+0x12>
   e:   00 ca 9a 3b 
  12:   c6 05 00 00 00 00 01    movb   $0x1,0(%rip)        # 19 <foo+0x19>
  19:   48 c7 05 00 00 00 00    movq   $0x3b9aca00,0(%rip)        # 24 <foo+0x24>
  20:   00 ca 9a 3b 
  24:   c6 05 00 00 00 00 01    movb   $0x1,0(%rip)        # 2b <foo+0x2b>
  2b:   c7 05 00 00 00 00 00    movl   $0xee6b2800,0(%rip)        # 35 <foo+0x35>
  32:   28 6b ee 
  35:   c7 05 00 00 00 00 00    movl   $0x0,0(%rip)        # 3f <foo+0x3f>
  3c:   00 00 00 
  3f:   c3                      retq   

One issue though is that currently using 'IF_Gbps(10)' triggers a warning
on 32-bit platforms.  I'd mitigate that by changing IF_Kbps to cast (x)
to a uintmax_t:

#define IF_Kbps(x)      ((uintmax_t)(x) * 1000) /* kilobits/sec. */

-- 
John Baldwin