Changeset View
Standalone View
sys/dev/ixgbe/if_ix.c
Show First 20 Lines • Show All 341 Lines • ▼ Show 20 Lines | |||||
SYSCTL_INT(_hw_ix, OID_AUTO, enable_fdir, CTLFLAG_RDTUN, &ixgbe_enable_fdir, 0, | SYSCTL_INT(_hw_ix, OID_AUTO, enable_fdir, CTLFLAG_RDTUN, &ixgbe_enable_fdir, 0, | ||||
"Enable Flow Director"); | "Enable Flow Director"); | ||||
/* Receive-Side Scaling */ | /* Receive-Side Scaling */ | ||||
static int ixgbe_enable_rss = 1; | static int ixgbe_enable_rss = 1; | ||||
SYSCTL_INT(_hw_ix, OID_AUTO, enable_rss, CTLFLAG_RDTUN, &ixgbe_enable_rss, 0, | SYSCTL_INT(_hw_ix, OID_AUTO, enable_rss, CTLFLAG_RDTUN, &ixgbe_enable_rss, 0, | ||||
"Enable Receive-Side Scaling (RSS)"); | "Enable Receive-Side Scaling (RSS)"); | ||||
/* | |||||
* AIM: Adaptive Interrupt Moderation | |||||
* which means that the interrupt rate | |||||
* is varied over time based on the | |||||
* traffic for that interrupt vector | |||||
*/ | |||||
static int ixgbe_enable_aim = FALSE; | |||||
SYSCTL_INT(_hw_ix, OID_AUTO, enable_aim, CTLFLAG_RWTUN, &ixgbe_enable_aim, 0, | |||||
"Enable adaptive interrupt moderation"); | |||||
#if 0 | #if 0 | ||||
/* Keep running tab on them for sanity check */ | /* Keep running tab on them for sanity check */ | ||||
static int ixgbe_total_ports; | static int ixgbe_total_ports; | ||||
#endif | #endif | ||||
MALLOC_DEFINE(M_IXGBE, "ix", "ix driver allocations"); | MALLOC_DEFINE(M_IXGBE, "ix", "ix driver allocations"); | ||||
/* | /* | ||||
▲ Show 20 Lines • Show All 1,722 Lines • ▼ Show 20 Lines | fail: | ||||
iflib_irq_free(ctx, &adapter->irq); | iflib_irq_free(ctx, &adapter->irq); | ||||
rx_que = adapter->rx_queues; | rx_que = adapter->rx_queues; | ||||
for (int i = 0; i < adapter->num_rx_queues; i++, rx_que++) | for (int i = 0; i < adapter->num_rx_queues; i++, rx_que++) | ||||
iflib_irq_free(ctx, &rx_que->que_irq); | iflib_irq_free(ctx, &rx_que->que_irq); | ||||
return (error); | return (error); | ||||
} /* ixgbe_if_msix_intr_assign */ | } /* ixgbe_if_msix_intr_assign */ | ||||
static inline void | |||||
ixgbe_perform_aim(struct adapter *adapter, struct ix_rx_queue *que) | |||||
{ | |||||
uint32_t newitr = 0; | |||||
struct rx_ring *rxr = &que->rxr; | |||||
/* | |||||
* Do Adaptive Interrupt Moderation: | |||||
* - Write out last calculated setting | |||||
* - Calculate based on average size over | |||||
* the last interval. | |||||
*/ | |||||
if (que->eitr_setting) { | |||||
IXGBE_WRITE_REG(&adapter->hw, IXGBE_EITR(que->msix), | |||||
que->eitr_setting); | |||||
} | |||||
que->eitr_setting = 0; | |||||
/* Idle, do nothing */ | |||||
if (rxr->bytes == 0) { | |||||
return; | |||||
} | |||||
if ((rxr->bytes) && (rxr->packets)) { | |||||
newitr = (rxr->bytes / rxr->packets); | |||||
} | |||||
newitr += 24; /* account for hardware frame, crc */ | |||||
/* set an upper boundary */ | |||||
newitr = min(newitr, 3000); | |||||
/* Be nice to the mid range */ | |||||
if ((newitr > 300) && (newitr < 1200)) { | |||||
newitr = (newitr / 3); | |||||
} else { | |||||
newitr = (newitr / 2); | |||||
} | |||||
if (adapter->hw.mac.type == ixgbe_mac_82598EB) { | |||||
newitr |= newitr << 16; | |||||
} else { | |||||
newitr |= IXGBE_EITR_CNT_WDIS; | |||||
} | |||||
/* save for next interrupt */ | |||||
que->eitr_setting = newitr; | |||||
/* Reset state */ | |||||
rxr->bytes = 0; | |||||
rxr->packets = 0; | |||||
return; | |||||
} | |||||
/********************************************************************* | /********************************************************************* | ||||
* ixgbe_msix_que - MSI-X Queue Interrupt Service routine | * ixgbe_msix_que - MSI-X Queue Interrupt Service routine | ||||
**********************************************************************/ | **********************************************************************/ | ||||
static int | static int | ||||
ixgbe_msix_que(void *arg) | ixgbe_msix_que(void *arg) | ||||
{ | { | ||||
struct ix_rx_queue *que = arg; | struct ix_rx_queue *que = arg; | ||||
struct adapter *adapter = que->adapter; | struct adapter *adapter = que->adapter; | ||||
struct ifnet *ifp = iflib_get_ifp(que->adapter->ctx); | struct ifnet *ifp = iflib_get_ifp(que->adapter->ctx); | ||||
/* Protect against spurious interrupts */ | /* Protect against spurious interrupts */ | ||||
if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) | if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) | ||||
return (FILTER_HANDLED); | return (FILTER_HANDLED); | ||||
ixgbe_disable_queue(adapter, que->msix); | ixgbe_disable_queue(adapter, que->msix); | ||||
++que->irqs; | ++que->irqs; | ||||
/* Check for AIM */ | |||||
if (adapter->enable_aim) { | |||||
ixgbe_perform_aim(adapter, que); | |||||
markj: Rather than a goto I think it would be nicer to lift this code into a separate function that… | |||||
Done Inline ActionsThis is old BSD11 code. In my opinion, I see no harm. New function in interrupt context may be little unwarranted overhead ? stallamr_netapp.com: This is old BSD11 code. In my opinion, I see no harm. New function in interrupt context may be… | |||||
Done Inline ActionsMost of the code in this function is for dealing with AIM, an optional feature. IMO it is simply easier to read the interrupt handler code if AIM code is in a separate function. It will be inlined by the compiler so there is no runtime difference. I don't think "it was this way before" is a good argument against making changes. markj: Most of the code in this function is for dealing with AIM, an optional feature. IMO it is… | |||||
} | |||||
return (FILTER_SCHEDULE_THREAD); | return (FILTER_SCHEDULE_THREAD); | ||||
} /* ixgbe_msix_que */ | } /* ixgbe_msix_que */ | ||||
/************************************************************************ | /************************************************************************ | ||||
* ixgbe_media_status - Media Ioctl callback | * ixgbe_media_status - Media Ioctl callback | ||||
* | * | ||||
* Called whenever the user queries the status of | * Called whenever the user queries the status of | ||||
* the interface using ifconfig. | * the interface using ifconfig. | ||||
************************************************************************/ | ************************************************************************/ | ||||
static void | static void | ||||
ixgbe_if_media_status(if_ctx_t ctx, struct ifmediareq * ifmr) | ixgbe_if_media_status(if_ctx_t ctx, struct ifmediareq * ifmr) | ||||
{ | { | ||||
struct adapter *adapter = iflib_get_softc(ctx); | struct adapter *adapter = iflib_get_softc(ctx); | ||||
struct ixgbe_hw *hw = &adapter->hw; | struct ixgbe_hw *hw = &adapter->hw; | ||||
int layer; | int layer; | ||||
INIT_DEBUGOUT("ixgbe_if_media_status: begin"); | INIT_DEBUGOUT("ixgbe_if_media_status: begin"); | ||||
Done Inline Actionsnewitr is always 0 here. I am nervous about the lack of atomicity with respect to rxr->packets and rxr->bytes. They are 32-bit values and could conceivably overflow, resulting in a division-by-zero. I would suggest loading the values with atomic_load_int() before doing any math with them, to ensure that you're operating on a consistent snapshot of the values. markj: `newitr` is always 0 here.
I am nervous about the lack of atomicity with respect to `rxr… | |||||
Done Inline ActionsWhy do you think newitr will be 0? Like in any possible case, we will have bytes >= packets. So, at the worst newitr be 1. I donot see how can it be 0. Next, these variables are u32. And we do disable IRQ , process rxq and then enable IRQ. For atomicity, these bytes/packets get increment when we process IRQ. So, the fact that we are in IRQ routine, that means these bytes/packets are a snapshot up-until previous IRQ. Like, I donot see possibility of race between IRQs and Rx processing. We do Rx processing with IRQ disabled. Also, there is no atomicity in BSD11 too. stallamr_netapp.com: Why do you think newitr will be 0?
Like in any possible case, we will have bytes >= packets. | |||||
Done Inline ActionsI mean, we have: newitr = 0; ... newitr = max(newitr, ...); so the max() is redundant. If it is possible for rxr->bytes / rxr->packets to be negative (I presume not), then this should be using imin() instead. With respect to the counters, I see that rxr->packets and rxr->bytes are incremented from iflib_rxeof(), which indeed disables queue interrupts, so I think you're right. This deserves a comment IMO. markj: I mean, we have:
```
newitr = 0;
...
newitr = max(newitr, ...);
```
so the `max()` is… | |||||
ifmr->ifm_status = IFM_AVALID; | ifmr->ifm_status = IFM_AVALID; | ||||
ifmr->ifm_active = IFM_ETHER; | ifmr->ifm_active = IFM_ETHER; | ||||
if (!adapter->link_active) | if (!adapter->link_active) | ||||
return; | return; | ||||
ifmr->ifm_status |= IFM_ACTIVE; | ifmr->ifm_status |= IFM_ACTIVE; | ||||
layer = adapter->phy_layer; | layer = adapter->phy_layer; | ||||
if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_T || | if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_T || | ||||
layer & IXGBE_PHYSICAL_LAYER_1000BASE_T || | layer & IXGBE_PHYSICAL_LAYER_1000BASE_T || | ||||
layer & IXGBE_PHYSICAL_LAYER_100BASE_TX || | layer & IXGBE_PHYSICAL_LAYER_100BASE_TX || | ||||
layer & IXGBE_PHYSICAL_LAYER_10BASE_T) | layer & IXGBE_PHYSICAL_LAYER_10BASE_T) | ||||
switch (adapter->link_speed) { | switch (adapter->link_speed) { | ||||
case IXGBE_LINK_SPEED_10GB_FULL: | case IXGBE_LINK_SPEED_10GB_FULL: | ||||
ifmr->ifm_active |= IFM_10G_T | IFM_FDX; | ifmr->ifm_active |= IFM_10G_T | IFM_FDX; | ||||
break; | break; | ||||
case IXGBE_LINK_SPEED_1GB_FULL: | case IXGBE_LINK_SPEED_1GB_FULL: | ||||
ifmr->ifm_active |= IFM_1000_T | IFM_FDX; | ifmr->ifm_active |= IFM_1000_T | IFM_FDX; | ||||
break; | break; | ||||
case IXGBE_LINK_SPEED_100_FULL: | case IXGBE_LINK_SPEED_100_FULL: | ||||
ifmr->ifm_active |= IFM_100_TX | IFM_FDX; | ifmr->ifm_active |= IFM_100_TX | IFM_FDX; | ||||
break; | break; | ||||
case IXGBE_LINK_SPEED_10_FULL: | case IXGBE_LINK_SPEED_10_FULL: | ||||
ifmr->ifm_active |= IFM_10_T | IFM_FDX; | ifmr->ifm_active |= IFM_10_T | IFM_FDX; | ||||
Done Inline ActionsI believe these counters may be modified concurrently. If so, these updates may be lost. Is that acceptable? If so I think it's worth a comment. markj: I believe these counters may be modified concurrently. If so, these updates may be lost. Is… | |||||
Done Inline ActionsNo, these counters donot get modified concurrently. Like said in above comment, we do disable IRQ - process Rx queue - re-enable IRQ. stallamr_netapp.com: No, these counters donot get modified concurrently. Like said in above comment, we do disable… | |||||
break; | break; | ||||
} | } | ||||
if (layer & IXGBE_PHYSICAL_LAYER_SFP_PLUS_CU || | if (layer & IXGBE_PHYSICAL_LAYER_SFP_PLUS_CU || | ||||
layer & IXGBE_PHYSICAL_LAYER_SFP_ACTIVE_DA) | layer & IXGBE_PHYSICAL_LAYER_SFP_ACTIVE_DA) | ||||
switch (adapter->link_speed) { | switch (adapter->link_speed) { | ||||
case IXGBE_LINK_SPEED_10GB_FULL: | case IXGBE_LINK_SPEED_10GB_FULL: | ||||
ifmr->ifm_active |= IFM_10G_TWINAX | IFM_FDX; | ifmr->ifm_active |= IFM_10G_TWINAX | IFM_FDX; | ||||
break; | break; | ||||
▲ Show 20 Lines • Show All 398 Lines • ▼ Show 20 Lines | SYSCTL_ADD_PROC(ctx_list, child, OID_AUTO, "fc", | ||||
CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | ||||
adapter, 0, ixgbe_sysctl_flowcntl, "I", | adapter, 0, ixgbe_sysctl_flowcntl, "I", | ||||
IXGBE_SYSCTL_DESC_SET_FC); | IXGBE_SYSCTL_DESC_SET_FC); | ||||
SYSCTL_ADD_PROC(ctx_list, child, OID_AUTO, "advertise_speed", | SYSCTL_ADD_PROC(ctx_list, child, OID_AUTO, "advertise_speed", | ||||
CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | ||||
adapter, 0, ixgbe_sysctl_advertise, "I", | adapter, 0, ixgbe_sysctl_advertise, "I", | ||||
IXGBE_SYSCTL_DESC_ADV_SPEED); | IXGBE_SYSCTL_DESC_ADV_SPEED); | ||||
adapter->enable_aim = ixgbe_enable_aim; | |||||
SYSCTL_ADD_INT(ctx_list, child, OID_AUTO, "enable_aim", CTLFLAG_RW, | |||||
&adapter->enable_aim, 0, "Interrupt Moderation"); | |||||
#ifdef IXGBE_DEBUG | #ifdef IXGBE_DEBUG | ||||
/* testing sysctls (for all devices) */ | /* testing sysctls (for all devices) */ | ||||
SYSCTL_ADD_PROC(ctx_list, child, OID_AUTO, "power_state", | SYSCTL_ADD_PROC(ctx_list, child, OID_AUTO, "power_state", | ||||
CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, | ||||
adapter, 0, ixgbe_sysctl_power_state, | adapter, 0, ixgbe_sysctl_power_state, | ||||
"I", "PCI Power State"); | "I", "PCI Power State"); | ||||
▲ Show 20 Lines • Show All 2,019 Lines • Show Last 20 Lines |
Rather than a goto I think it would be nicer to lift this code into a separate function that gets called here.