* Re: [net-next 8/9] ixgbe: add interface to export thermal data
From: Francois Romieu @ 2011-12-23 20:45 UTC (permalink / raw)
To: Skidmore, Donald C
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com, Waskiewicz Jr, Peter P
In-Reply-To: <F6FB0E698C9B3143BDF729DF22286646E087@ORSMSX102.amr.corp.intel.com>
Skidmore, Donald C <donald.c.skidmore@intel.com> :
[...]
> I like all your suggestions and will do the clean up.
>
> Thanks again for the great review. :)
Depending on how you plan to use ixgbe_init_thermal_sensor_thresh_generic,
the memset(..., 0, ...) may be removed since it is applied within the
device own kzalloced struct ixgbe_hw.
I must confess I have not looked too closely at the sysfs code.
--
Ueimor
^ permalink raw reply
* Re: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor
From: Francois Romieu @ 2011-12-23 20:43 UTC (permalink / raw)
To: Skidmore, Donald C
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com, Waskiewicz Jr, Peter P
In-Reply-To: <F6FB0E698C9B3143BDF729DF22286646E06B@ORSMSX102.amr.corp.intel.com>
Skidmore, Donald C <donald.c.skidmore@intel.com> :
[...]
> >While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-
> >next
> >and the remaining patches of this series. Can't you remove it completely
> >and
> >use a standard error code, say -EOPNOTSUPP ?
>
> Previously we have only been using EOPNOTSUPP and it's like as a return
> value seen outside the driver particularly to user space. Looking through
> a few other device drivers (in no way an extensive search) they 'seem' to
> be following that model too. Still that said I'm not aware of this being
> a BKM it's just what I noticed.
Your call. As long as the code does not end cluttered with:
rc = (status != MY_OWN_PRIVATE_ERROR_STATUS) ? 0 : -Esomething;
[...]
> if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) != IXGBE_ETS_TYPE_EMC) {
>
> Or are you talking about just moving the "!=" to the previous line?
Just the "!=".
You may include the shift in the definition of IXGBE_ETS_TYPE_EMC itself
so that it disappears here.
[...]
> While I like this idea and it would remove some duplicate code, I need
> to use ets_cfg in ixgbe_init_thermal_sensor_thresh_generic() to get the
> low_thresh_delta.
You are right, I missed it.
> This makes the support function a little less useful as it would either
> need to return two values (num_sensors and low_thresh_delta) or just the
> ets_cfs itself. For the latter case the support function would just be
> making two reads and we would still need the local variables. Still might
> be worth it though.
Something like a stack allocated struct ets { u16 cfg; u16 sensor; } may
help.
--
Ueimor
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: David Miller @ 2011-12-23 19:36 UTC (permalink / raw)
To: shemminger; +Cc: eric.dumazet, dave.taht, jsullivan, netdev
In-Reply-To: <20111223110749.7a690685@nehalam.linuxnetplumber.net>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Fri, 23 Dec 2011 11:07:49 -0800
> So basically, netem, choke, and sfb are incompatible with
> each other. This is not that bad, why not add a flag to qdisc
> ops to indicate which qdisc are using cb and block user from
> trying to do something bogus.
Or we could do what we do in the inet stack, define a layout
that allows the different layers to use different parts of the
CB.
^ permalink raw reply
* Re: [net-next] stmmac: fix missing module license in the main.
From: David Miller @ 2011-12-23 19:21 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1324636267-25533-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Fri, 23 Dec 2011 11:31:07 +0100
> This patch fixes the following warning raised
> when compile:
>
> WARNING: modpost: missing MODULE_LICENSE()
> in drivers/net/ethernet/stmicro/stmmac/stmmac.o
>
> Reported-by: Randy Dunlap <rdunlap@xenotime.net>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Applied.
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Eric Dumazet @ 2011-12-23 19:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Dave Taht, John A. Sullivan III, netdev
In-Reply-To: <20111223110749.7a690685@nehalam.linuxnetplumber.net>
Le vendredi 23 décembre 2011 à 11:07 -0800, Stephen Hemminger a écrit :
> skb_cb is the dumping ground of the networking layer.
> The assumption was that the qdisc could use the skb_cb
> for it's own scratchpad. Netem is using it for tagging
> packets in the queue.
>
> So basically, netem, choke, and sfb are incompatible with
> each other. This is not that bad, why not add a flag to qdisc
> ops to indicate which qdisc are using cb and block user from
> trying to do something bogus.
This is not how I planned to solve the problem.
I think we need an internal tfifo for netem use.
Then be able to add another qdisc on top of netem.
tfifo as a first stage, hardcoded (only limit is tunable)
[ optional 2nd stage, any qdisc ]
netem_queue()
{
queue packet to tfifo
(eventually at head, of reordering)
}
netem_dequeue()
{
if (other_qdisc) {
for_each_packet_from_tfifo_time_ready() {
other_qdisc->enqueue(skb);
}
try_to_dequeue_one_packet_from(other_qdisc);
} else {
dequeue_one_packet_from_tfifo_time_ready();
}
}
^ permalink raw reply
* netem: loss model API sizes
From: Stephen Hemminger @ 2011-12-23 19:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev
The new netem loss model is configured with nested netlink messages.
This code is being overly strict about sizes, and is easily confused
by padding (or possible future expansion). Also message
for gemodel is incorrect.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/net/sched/sch_netem.c 2011-08-01 14:13:55.000000000 -0700
+++ b/net/sched/sch_netem.c 2011-12-22 16:57:35.310210173 -0800
@@ -548,7 +548,7 @@ static int get_loss_clg(struct Qdisc *sc
case NETEM_LOSS_GI: {
const struct tc_netem_gimodel *gi = nla_data(la);
- if (nla_len(la) != sizeof(struct tc_netem_gimodel)) {
+ if (nla_len(la) < sizeof(struct tc_netem_gimodel)) {
pr_info("netem: incorrect gi model size\n");
return -EINVAL;
}
@@ -567,8 +567,8 @@ static int get_loss_clg(struct Qdisc *sc
case NETEM_LOSS_GE: {
const struct tc_netem_gemodel *ge = nla_data(la);
- if (nla_len(la) != sizeof(struct tc_netem_gemodel)) {
- pr_info("netem: incorrect gi model size\n");
+ if (nla_len(la) < sizeof(struct tc_netem_gemodel)) {
+ pr_info("netem: incorrect ge model size\n");
return -EINVAL;
}
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Stephen Hemminger @ 2011-12-23 19:07 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Dave Taht, John A. Sullivan III, netdev
In-Reply-To: <1324664907.2915.5.camel@edumazet-laptop>
On Fri, 23 Dec 2011 19:28:27 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 23 décembre 2011 à 18:54 +0100, Dave Taht a écrit :
>
> > Are there any place where all 48 bytes of cb are used?
> >
>
> Yes, but on in qdisc layer.
>
> struct tcp_skb_cb is known to be 44 bytes (when IPv6 is enabled)
>
> In qdisc layer, we use a small part of it, for the moment.
>
> > I wouldn't mind if 'time_to_send' became a separate skb field
> > for a more generic 'time_in_queue'...
> >
>
> This wont happen.
>
> As I posted in an earlier patch, this can be added in "struct
> qdisc_skb_cb"
>
>
>
skb_cb is the dumping ground of the networking layer.
The assumption was that the qdisc could use the skb_cb
for it's own scratchpad. Netem is using it for tagging
packets in the queue.
So basically, netem, choke, and sfb are incompatible with
each other. This is not that bad, why not add a flag to qdisc
ops to indicate which qdisc are using cb and block user from
trying to do something bogus.
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Dave Taht @ 2011-12-23 18:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, John A. Sullivan III, netdev
In-Reply-To: <1324664907.2915.5.camel@edumazet-laptop>
On Fri, Dec 23, 2011 at 7:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 23 décembre 2011 à 18:54 +0100, Dave Taht a écrit :
>
>> Are there any place where all 48 bytes of cb are used?
>>
>
> Yes, but on in qdisc layer.
>
> struct tcp_skb_cb is known to be 44 bytes (when IPv6 is enabled)
>
> In qdisc layer, we use a small part of it, for the moment.
>
>> I wouldn't mind if 'time_to_send' became a separate skb field
>> for a more generic 'time_in_queue'...
>>
>
> This wont happen.
>
> As I posted in an earlier patch, this can be added in "struct
> qdisc_skb_cb"
Ah... this patch?
http://patchwork.ozlabs.org/patch/125329/
I liked what I saw then, but nobody chirped up to review it...
I guess I know what I'm doing this weekend.
>
>
>
--
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
FR Tel: 0638645374
http://www.bufferbloat.net
^ permalink raw reply
* RE: [net-next 8/9] ixgbe: add interface to export thermal data
From: Skidmore, Donald C @ 2011-12-23 18:29 UTC (permalink / raw)
To: Francois Romieu, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com, Waskiewicz Jr, Peter P
In-Reply-To: <20111223130148.GB27206@electric-eye.fr.zoreil.com>
>-----Original Message-----
>From: Francois Romieu [mailto:romieu@fr.zoreil.com]
>Sent: Friday, December 23, 2011 5:02 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
>
>Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some of our adapters have thermal data available, this patch exports
>this
>> data via a read-only sysfs interface. More patches will follow that
>will
>> contain additional information to be exported.
>>
>> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/Makefile | 2 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c | 2 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c | 305
>++++++++++++++++++++++++
>[...]
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> index 7720721..1a3810e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599
>= {
>> .set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>> .acquire_swfw_sync = &ixgbe_acquire_swfw_sync,
>> .release_swfw_sync = &ixgbe_release_swfw_sync,
>> + .get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
>> + .init_thermal_sensor_thresh =
>&ixgbe_init_thermal_sensor_thresh_generic,
>
>.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
>be read anywhere.
>
>>
>> };
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index e27e4d1..b5cef7e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>>
>> e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>> cards_found++;
>> +
>> + if (ixgbe_sysfs_init(adapter))
>> + e_err(probe, "failed to allocate sysfs resources\n");
>> +
>> return 0;
>>
>> err_register:
>> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct
>pci_dev *pdev)
>> }
>>
>> #endif
>> + ixgbe_sysfs_exit(adapter);
>> +
>> #ifdef IXGBE_FCOE
>> if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>> ixgbe_cleanup_fcoe(adapter);
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>> new file mode 100644
>> index 0000000..db818ae
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>[...]
>> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)
>
>Please name it ixgbe_sysfs_del_adapter or anything better.
>
>> +{
>> + int i;
>> +
>> + if (adapter == NULL)
>> + return;
>> +
>> + for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>> + if (adapter->therm_kobj[i] == NULL)
>> + continue;
>> + sysfs_remove_group(adapter->therm_kobj[i],
>&therm_attr_group);
>> + kobject_put(adapter->therm_kobj[i]);
>> + }
>> + if (adapter->info_kobj != NULL)
>> + kobject_put(adapter->info_kobj);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
>> +{
>> + ixgbe_del_adapter(adapter);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
>> +{
>> + struct net_device *netdev;
>> + int rc = 0;
>> + int i;
>> + char buf[16];
>> +
>> + if (adapter == NULL)
>> + goto err;
>
>ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter
>can
>not be NULL.
>
>> + netdev = adapter->netdev;
>> + if (netdev == NULL)
>> + goto err;
>
>netdev can not be NULL.
>
>> +
>> + adapter->info_kobj = NULL;
>> + for (i = 0; i < IXGBE_MAX_SENSORS; i++)
>> + adapter->therm_kobj[i] = NULL;
>> +
>> + /* create info kobj and attribute listings in kobj */
>> + adapter->info_kobj = kobject_create_and_add("info",
>> + &(netdev->dev.kobj));
>
>Remove comment and parenthesis. Everything can fit in a single line.
>
>> + if (adapter->info_kobj == NULL)
>> + goto err;
>> +
>> + /* Don't create thermal subkobjs if no data present */
>> + if (ixgbe_thermal_present(adapter->info_kobj) != true)
>> + goto exit;
>> +
>> + for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>
>'buf' ought to be declared here.
>
>> +
>> + /*
>> + * Likewise only create individual kobjs that have
>> + * meaningful data.
>> + */
>> + if (adapter->hw.mac.thermal_sensor_data.sensor[i].location
>== 0)
>> + continue;
>> +
>> + /* directory named after sensor offset */
>> + snprintf(buf, sizeof(buf), "sensor_%d", i);
>> + adapter->therm_kobj[i] =
>> + kobject_create_and_add(buf, adapter->info_kobj);
>> + if (adapter->therm_kobj[i] == NULL)
>> + goto err;
>> + if (sysfs_create_group(adapter->therm_kobj[i],
>> + &therm_attr_group))
>> + goto err;
>> + }
>> +
>> + goto exit;
>> +
>> +err:
>> + ixgbe_del_adapter(adapter);
>> + rc = -1;
>> +exit:
>> + return rc;
>> +}
>> +
>> --
>> 1.7.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
I like all your suggestions and will do the clean up.
Thanks again for the great review. :)
-Don Skidmore <donald.c.skidmore@intel.com>
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Eric Dumazet @ 2011-12-23 18:28 UTC (permalink / raw)
To: Dave Taht; +Cc: Stephen Hemminger, John A. Sullivan III, netdev
In-Reply-To: <CAA93jw77tL88oxt=2ZTs68k5awCOO2f4d+aE9H-yQdTsd8=gAQ@mail.gmail.com>
Le vendredi 23 décembre 2011 à 18:54 +0100, Dave Taht a écrit :
> Are there any place where all 48 bytes of cb are used?
>
Yes, but on in qdisc layer.
struct tcp_skb_cb is known to be 44 bytes (when IPv6 is enabled)
In qdisc layer, we use a small part of it, for the moment.
> I wouldn't mind if 'time_to_send' became a separate skb field
> for a more generic 'time_in_queue'...
>
This wont happen.
As I posted in an earlier patch, this can be added in "struct
qdisc_skb_cb"
^ permalink raw reply
* RE: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor
From: Skidmore, Donald C @ 2011-12-23 18:27 UTC (permalink / raw)
To: Francois Romieu, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com, Waskiewicz Jr, Peter P
In-Reply-To: <20111223130107.GA27206@electric-eye.fr.zoreil.com>
>-----Original Message-----
>From: Francois Romieu [mailto:romieu@fr.zoreil.com]
>Sent: Friday, December 23, 2011 5:01 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 7/9] ixgbe: add support functions for gathering
>thermal data sensor
>
>Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some 82599 adapters contain thermal data that we can get to via
>> an i2c interface. These functions provide support to get at that
>> data. A following patch will export this data.
>>
>> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 150
>+++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 13 ++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 38 ++++++
>> 3 files changed, 201 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> index a3aa633..05aa156 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>> @@ -3526,3 +3526,153 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw
>*hw)
>> IXGBE_WRITE_REG(hw, IXGBE_GCR_EXT, gcr_ext);
>> IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
>> }
>> +
>> +static const u8 ixgbe_emc_temp_data[4] = {
>> + IXGBE_EMC_INTERNAL_DATA,
>> + IXGBE_EMC_DIODE1_DATA,
>> + IXGBE_EMC_DIODE2_DATA,
>> + IXGBE_EMC_DIODE3_DATA
>> +};
>> +static const u8 ixgbe_emc_therm_limit[4] = {
>> + IXGBE_EMC_INTERNAL_THERM_LIMIT,
>> + IXGBE_EMC_DIODE1_THERM_LIMIT,
>> + IXGBE_EMC_DIODE2_THERM_LIMIT,
>> + IXGBE_EMC_DIODE3_THERM_LIMIT
>> +};
>> +
>> +/**
>> + * ixgbe_get_thermal_sensor_data - Gathers thermal sensor data
>> + * @hw: pointer to hardware structure
>> + * @data: pointer to the thermal sensor data structure
>> + *
>> + * Returns the thermal sensor data structure
>> + **/
>> +s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw)
>> +{
>> + s32 status = 0;
>> + u16 ets_offset;
>> + u16 ets_cfg;
>> + u16 ets_sensor;
>> + u8 num_sensors;
>> + u8 sensor_index;
>> + u8 sensor_location;
>> + u8 i;
>> + struct ixgbe_thermal_sensor_data *data = &hw-
>>mac.thermal_sensor_data;
>> +
>> + /* Only support thermal sensors attached to 82599 physical port 0
>*/
>> + if ((hw->mac.type != ixgbe_mac_82599EB) ||
>> + (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)) {
>> + status = IXGBE_NOT_IMPLEMENTED;
>
>While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-
>next
>and the remaining patches of this series. Can't you remove it completely
>and
>use a standard error code, say -EOPNOTSUPP ?
Previously we have only been using EOPNOTSUPP and it's like as a return value seen outside the driver particularly to user space. Looking through a few other device drivers (in no way an extensive search) they 'seem' to be following that model too. Still that said I'm not aware of this being a BKM it's just what I noticed.
>
>> + goto out;
>> + }
>> +
>> + status = hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
>> + if (status)
>> + goto out;
>> +
>> + if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF)) {
>> + status = IXGBE_NOT_IMPLEMENTED;
>> + goto out;
>> + }
>> +
>> + status = hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
>> + if (status)
>> + goto out;
>> +
>> + if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
>> + != IXGBE_ETS_TYPE_EMC) {
> ^^ -> should appear in the previous line
>
I'm probably just missing your point, but if I added this to the previous line I would be over 80 chars?
if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) != IXGBE_ETS_TYPE_EMC) {
Or are you talking about just moving the "!=" to the previous line?
if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT) !=
IXGBE_ETS_TYPE_EMC) {
I have a strong feeling I'm just not getting what you're suggesting here. Please clue me in.
>> + status = IXGBE_NOT_IMPLEMENTED;
>> + goto out;
>> + }
>> +
>> + num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
>> + if (num_sensors > IXGBE_MAX_SENSORS)
>> + num_sensors = IXGBE_MAX_SENSORS;
>
>ets_cfg is not used beyond this point and most of this code is
>duplicated
>in .
>
>You may refactor the code with xyz_get_ets_offset() and
>xyz_get_num_sensors()
>methods, add some eth_type local variable and avoid the duplication
>while
>keeping the lines short.
>
While I like this idea and it would remove some duplicate code, I need to use ets_cfg in ixgbe_init_thermal_sensor_thresh_generic() to get the low_thresh_delta. This makes the support function a little less useful as it would either need to return two values (num_sensors and low_thresh_delta) or just the ets_cfs itself. For the latter case the support function would just be making two reads and we would still need the local variables. Still might be worth it though.
>> +
>> + for (i = 0; i < num_sensors; i++) {
>
>sensor_index and sensor_location should be declared here.
>
Agreed. :)
>> + status = hw->eeprom.ops.read(hw, (ets_offset + 1 + i),
>> + &ets_sensor);
>> + if (status)
>> + goto out;
>> +
>> + sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
>> + IXGBE_ETS_DATA_INDEX_SHIFT);
>> + sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
>> + IXGBE_ETS_DATA_LOC_SHIFT);
>> +
>> + if (sensor_location != 0) {
>> + status = hw->phy.ops.read_i2c_byte(hw,
>> + ixgbe_emc_temp_data[sensor_index],
>> + IXGBE_I2C_THERMAL_SENSOR_ADDR,
>> + &data->sensor[i].temp);
>> + if (status)
>> + goto out;
>> + }
>
>Broken indentation.
Glad you noticed this I will correct it.
>
>> + }
>> +out:
>> + return status;
>> +}
>> +
>> +/**
>> + * ixgbe_init_thermal_sensor_thresh_generic - Inits thermal sensor
>thresholds
>> + * @hw: pointer to hardware structure
>> + *
>> + * Inits the thermal sensor thresholds according to the NVM map
>> + * and save off the threshold and location values into
>mac.thermal_sensor_data
>> + **/
>> +s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw)
>> +{
>> + s32 status = 0;
>> + u16 ets_offset;
>> + u16 ets_cfg;
>> + u16 ets_sensor;
>> + u8 low_thresh_delta;
>> + u8 num_sensors;
>> + u8 sensor_index;
>> + u8 sensor_location;
>> + u8 therm_limit;
>> + u8 i;
>> + struct ixgbe_thermal_sensor_data *data = &hw-
>>mac.thermal_sensor_data;
>> +
>> + memset(data, 0, sizeof(struct ixgbe_thermal_sensor_data));
>> +
>> + /* Only support thermal sensors attached to 82599 physical port 0
>*/
>> + if ((hw->mac.type != ixgbe_mac_82599EB) ||
>> + (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1))
>> + return IXGBE_NOT_IMPLEMENTED;
>> +
>> + hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
>> + if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF))
>> + return IXGBE_NOT_IMPLEMENTED;
>> +
>> + hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
>> + if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
>> + != IXGBE_ETS_TYPE_EMC)
> ^^ sic
>
Like I mentioned about I don't think I understand what you're pointing out here.
>> + return IXGBE_NOT_IMPLEMENTED;
>> +
>> + low_thresh_delta = ((ets_cfg & IXGBE_ETS_LTHRES_DELTA_MASK) >>
>> + IXGBE_ETS_LTHRES_DELTA_SHIFT);
>> + num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
>> +
>> + for (i = 0; i < num_sensors; i++) {
>> + hw->eeprom.ops.read(hw, (ets_offset + 1 + i), &ets_sensor);
>> + sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
>> + IXGBE_ETS_DATA_INDEX_SHIFT);
>> + sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
>> + IXGBE_ETS_DATA_LOC_SHIFT);
>> + therm_limit = ets_sensor & IXGBE_ETS_DATA_HTHRESH_MASK;
>> +
>> + hw->phy.ops.write_i2c_byte(hw,
>> + ixgbe_emc_therm_limit[sensor_index],
>> + IXGBE_I2C_THERMAL_SENSOR_ADDR, therm_limit);
>> +
>> + if ((i < IXGBE_MAX_SENSORS) && (sensor_location != 0)) {
>
>The patch would be easier to review if num_sensors was capped as in
>ixgbe_get_thermal_sensor_data_generic...
>
>> + data->sensor[i].location = sensor_location;
>> + data->sensor[i].caution_thresh = therm_limit;
>> + data->sensor[i].max_op_thresh = therm_limit -
>> + low_thresh_delta;
>
>... and the code would gain an extra tab level, thus avoiding the line
>break if there was a 'continue' statement when sensor_location fails the
>test.
I like this too and will update the patch.
Thanks for review I do really appreciate it.
-Don Skidmore <donald.c.skidmore@intel.com>
>
>--
>Ueimor
^ permalink raw reply
* Re: [PATCH 0/4] skb paged fragment destructors
From: Michał Mirosław @ 2011-12-23 18:10 UTC (permalink / raw)
To: David Miller; +Cc: Ian.Campbell, eric.dumazet, jesse.brandeburg, netdev
In-Reply-To: <20111222.143422.1948369460874203875.davem@davemloft.net>
2011/12/22 David Miller <davem@davemloft.net>:
> From: Michał Mirosław <mirqus@gmail.com>
> Date: Thu, 22 Dec 2011 20:29:30 +0100
>
>> 2011/12/22 David Miller <davem@davemloft.net>:
>>> From: Michał Mirosław <mirqus@gmail.com>
>>> Date: Thu, 22 Dec 2011 19:34:21 +0100
>>>
>>>> 2011/12/22 Ian Campbell <Ian.Campbell@citrix.com>:
>>>>> On Wed, 2011-12-21 at 19:28 +0000, David Miller wrote:
>>>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>>>> Date: Wed, 21 Dec 2011 15:02:18 +0100
>>>>>> > No idea on this +2 point.
>>>>>> I think I know, and I believe I instructed Alexey Kuznetsov to do
>>>>>> this.
>>>>>>
>>>>>> When sendfile() is performed, we might start the SKB with the last few
>>>>>> bytes of one page, and end the SKB with the first few bytes of another
>>>>>> page.
>>>>>>
>>>>>> In order to fit a full 64K frame into an SKB in this situation we have
>>>>>> to accomodate this case.
>>>>> Thanks David, that makes sense.
>>>>>
>>>>> However I think you only actually need 1 extra page for that. If the
>>>>> data in frag[0] starts at $offset then frag[16] will need to have
>>>>> $offset bytes in it. e.g.
>>>>> 4096-$offset + 4096*15 + $offset = 65536
>>>>> which == 17 pages rather than 18.
>>>>>
>>>>> The following documents the status quo but I could update to switch to +
>>>>> 1 instead if there are no flaws in the above logic...
>>>>
>>>> Since max IP datagram is 64K-1, adding ethernet and possibly VLAN
>>>> headers makes the max size slightly above 64K and then you have
>>>> 64K/PAGE_SIZE+2 pages appear in worst case.
>>>
>>> Headers go into the linear area, so they are not relevant for these
>>> calculations.
>>
>> Does this hold for LRO'ed packets and packets sent via PF_PACKET socket?
>
> LRO is for receive, not packets we build on transmit, and in any event
> have their headers also pulled into the linear area before the stack
> sees it.
Yes, but the drivers seem to first fill the data to frags[] and only
then move the packets header (I browsed myri10ge).
> For PF_PACKET, in the packet_snd() case it uses a linear SKB and in
> the tpacket_snd() case it uses a linear SKB as well.
I saw skb_fill_page_desc() in af_packet.c (called by
tpacket_fill_skb() which is called by tpacket_snd()), that's why I'm
asking.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [net-next 8/9] ixgbe: add interface to export thermal data
From: Michał Mirosław @ 2011-12-23 17:58 UTC (permalink / raw)
To: Jeff Kirsher
Cc: davem, Don Skidmore, netdev, gospo, sassmann,
Peter P Waskiewicz Jr
In-Reply-To: <1324631357-31789-9-git-send-email-jeffrey.t.kirsher@intel.com>
2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> From: Don Skidmore <donald.c.skidmore@intel.com>
>
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.
Just curious: can't this use the hwmon subsystem to be consistent with
other system monitoring devices?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Dave Taht @ 2011-12-23 17:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, John A. Sullivan III, netdev
In-Reply-To: <1324661950.4540.20.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
On Fri, Dec 23, 2011 at 6:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 23 décembre 2011 à 18:33 +0100, Eric Dumazet a écrit :
>> Le dimanche 18 décembre 2011 à 11:55 -0800, Stephen Hemminger a écrit :
>> > On Sun, 18 Dec 2011 00:12:12 -0500
>> > "John A. Sullivan III" <jsullivan@opensourcedevel.com> wrote:
>> >
>> > > Since netem appears to be classless, we realized we would need to
>> > > replace the SFQ on each leaf with netem which we really didn't want to
>> > > do - not only to not lose SFQ but because we didn't want to maintain the
>> > > netem parameters on each leaf. So, we activated our ifb1 interface,
>> > > placed netem on it and redirected all the egress traffic to ifb1. Taht
>> > > worked fine.
>> >
>> > Current versions of netem can take one class.
>> > --
>>
>> Hmm, I can see that (commit 10f6dfcfde884441)
>>
>> But it wont work very well, it assumes qdisc uses a single queue if
>> netem reordering is requested :
>>
>> (__skb_queue_head(&q->qdisc->q, skb))
>>
>> We should allow reordering if netem queue is changed from tfifo, only if
>> new qdisc is compatible with __skb_queue_head(&q->qdisc->q, skb)
>>
>> (maybe providing a new ->queue_at_head() new ops)
>
> Also, child qdisc must not scratch skb->cb[], since netem stores
> time_to_send in it.
Are there any place where all 48 bytes of cb are used?
I wouldn't mind if 'time_to_send' became a separate skb field
for a more generic 'time_in_queue'...
> I guess nobody actually tried this netem mis-feature :(
>
> I'll send a fix.
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
FR Tel: 0638645374
http://www.bufferbloat.net
^ permalink raw reply
* [PATCH 2/9] net/rfkill/rfkill-gpio.c: introduce missing kfree
From: Julia Lawall @ 2011-12-23 17:39 UTC (permalink / raw)
To: John W. Linville
Cc: kernel-janitors, Johannes Berg, David S. Miller, linux-wireless,
netdev, linux-kernel
Error handling code following a kmalloc should free the allocated data.
The label fail_alloc already does this for rfkill.
A simplified version of the semantic match that finds the problem is as
follows: (http://coccinelle.lip6.fr)
// <smpl>
@r exists@
local idexpression x;
statement S;
identifier f1;
position p1,p2;
expression *ptr != NULL;
@@
x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...x...+> }
x->f1
...>
(
return \(0\|<+...x...+>\|ptr\);
|
return@p2 ...;
)
@script:python@
p1 << r.p1;
p2 << r.p2;
@@
print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
net/rfkill/rfkill-gpio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index ca355e7..865adb6 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -105,7 +105,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
ret = pdata->gpio_runtime_setup(pdev);
if (ret) {
pr_warn("%s: can't set up gpio\n", __func__);
- return ret;
+ goto fail_alloc;
}
}
^ permalink raw reply related
* Re: netem and hierarchical ingress traffic shaping
From: Eric Dumazet @ 2011-12-23 17:39 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: John A. Sullivan III, netdev
In-Reply-To: <1324661639.4540.18.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
Le vendredi 23 décembre 2011 à 18:33 +0100, Eric Dumazet a écrit :
> Le dimanche 18 décembre 2011 à 11:55 -0800, Stephen Hemminger a écrit :
> > On Sun, 18 Dec 2011 00:12:12 -0500
> > "John A. Sullivan III" <jsullivan@opensourcedevel.com> wrote:
> >
> > > Since netem appears to be classless, we realized we would need to
> > > replace the SFQ on each leaf with netem which we really didn't want to
> > > do - not only to not lose SFQ but because we didn't want to maintain the
> > > netem parameters on each leaf. So, we activated our ifb1 interface,
> > > placed netem on it and redirected all the egress traffic to ifb1. Taht
> > > worked fine.
> >
> > Current versions of netem can take one class.
> > --
>
> Hmm, I can see that (commit 10f6dfcfde884441)
>
> But it wont work very well, it assumes qdisc uses a single queue if
> netem reordering is requested :
>
> (__skb_queue_head(&q->qdisc->q, skb))
>
> We should allow reordering if netem queue is changed from tfifo, only if
> new qdisc is compatible with __skb_queue_head(&q->qdisc->q, skb)
>
> (maybe providing a new ->queue_at_head() new ops)
Also, child qdisc must not scratch skb->cb[], since netem stores
time_to_send in it.
I guess nobody actually tried this netem mis-feature :(
I'll send a fix.
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 17:35 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324661598.10184.615.camel@denise.theartistscloset.com>
On Fri, 2011-12-23 at 12:33 -0500, John A. Sullivan III wrote:
> On Fri, 2011-12-23 at 18:17 +0100, Eric Dumazet wrote:
> > Le vendredi 23 décembre 2011 à 18:06 +0100, Eric Dumazet a écrit :
> >
> > > Maybe I was not clear :
> > >
> > > netem currently uses a fifo queue, you cant change this, without
> > > patching kernel.
> > >
> >
> > An other way would be to patch sch_tbf, adding delay, if its all you
> > want to do.
> >
> > (Or adding delay capability to ifb)
> >
> >
> >
> >
> <grin> that's just a little outside by skill set ;) - John
>
<snip>
And I should mention seriously, that we are viewing our learning curve
as something we will use in production so we'd like to accomplish our
goals using stock distribution code. Thanks, though - John
^ permalink raw reply
* Re: netem and hierarchical ingress traffic shaping
From: Eric Dumazet @ 2011-12-23 17:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: John A. Sullivan III, netdev
In-Reply-To: <20111218115549.0d9e1259@nehalam.linuxnetplumber.net>
Le dimanche 18 décembre 2011 à 11:55 -0800, Stephen Hemminger a écrit :
> On Sun, 18 Dec 2011 00:12:12 -0500
> "John A. Sullivan III" <jsullivan@opensourcedevel.com> wrote:
>
> > Since netem appears to be classless, we realized we would need to
> > replace the SFQ on each leaf with netem which we really didn't want to
> > do - not only to not lose SFQ but because we didn't want to maintain the
> > netem parameters on each leaf. So, we activated our ifb1 interface,
> > placed netem on it and redirected all the egress traffic to ifb1. Taht
> > worked fine.
>
> Current versions of netem can take one class.
> --
Hmm, I can see that (commit 10f6dfcfde884441)
But it wont work very well, it assumes qdisc uses a single queue if
netem reordering is requested :
(__skb_queue_head(&q->qdisc->q, skb))
We should allow reordering if netem queue is changed from tfifo, only if
new qdisc is compatible with __skb_queue_head(&q->qdisc->q, skb)
(maybe providing a new ->queue_at_head() new ops)
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 17:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324660666.4540.14.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
On Fri, 2011-12-23 at 18:17 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 18:06 +0100, Eric Dumazet a écrit :
>
> > Maybe I was not clear :
> >
> > netem currently uses a fifo queue, you cant change this, without
> > patching kernel.
> >
>
> An other way would be to patch sch_tbf, adding delay, if its all you
> want to do.
>
> (Or adding delay capability to ifb)
>
>
>
>
<grin> that's just a little outside by skill set ;) - John
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 17:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324659976.4540.12.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
On Fri, 2011-12-23 at 18:06 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 11:44 -0500, John A. Sullivan III a
> écrit :
> > On Fri, 2011-12-23 at 17:16 +0100, Eric Dumazet wrote:
> > > Le vendredi 23 décembre 2011 à 10:26 -0500, John A. Sullivan III a
> > > écrit :
> > >
> > > > Yes. This is a problem I have with netem on ingress traffic. I use the
> > > > filter on ffff: to redirect to ifb0 for the ingress traffic shaping. I
> > > > cannot figure out a way to redirect a second time to ifb1 for the netem
> > > > qdisc. I tried putting two action mirred statements in the filter but
> > > > that did not work. Unlike eth1, I cannot attach a filter further down
> > > > the ifb0 hfsc hierarchy because one can't redirect one ifb into another
> > > > ifb. Thus, the only way I could figure out how to do inbound netem was
> > > > to replace the terminal qdisc with netem rather than SFQ. I'd love to
> > > > be able to do that differently. I tried attaching netem to the SFQ but
> > > > that failed (I assume because SFQ is classless) and I tried the other
> > > > way around, attaching SFQ to netem since you mentioned netem could take
> > > > a class but that did not work either.
> > >
> > > Unfortunately, netem wants to control skbs itself, in a fifo queue.
> > >
> > > To implement what you want, we would need to setup a second qdisc,
> > > and when packets are dequeued from internal netem fifo, queue them in
> > > second qdisc.
> > >
> > >
> > >
> > I thought I tried to do that but I must have done it incorrectly. I
> > would think something like:
> >
> > tc qdisc add dev eth1 ingress
> > tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0 0 action mirred egress redirect dev ifb0
> > tc qdisc add dev ifb0 root handle 4 netem delay 25ms 5ms distribution normal loss 0.1% 30%
> > tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
> >
> > but I get:
> > root@testswitch01:~# tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
> > RTNETLINK answers: Operation not supported
> >
> > What did I do wrong? Thanks - John
> >
>
> Maybe I was not clear :
>
> netem currently uses a fifo queue, you cant change this, without
> patching kernel.
>
>
>
OK - that makes sense and explains what I was seeing but, once they are
out the netem fifo, aren't they headed for the NIC driver? How do we
queue them in a second qdisc? I'll gladly give it a try if I know how.
Thanks - John
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 17:17 UTC (permalink / raw)
To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324659976.4540.12.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
Le vendredi 23 décembre 2011 à 18:06 +0100, Eric Dumazet a écrit :
> Maybe I was not clear :
>
> netem currently uses a fifo queue, you cant change this, without
> patching kernel.
>
An other way would be to patch sch_tbf, adding delay, if its all you
want to do.
(Or adding delay capability to ifb)
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 17:06 UTC (permalink / raw)
To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324658658.10184.613.camel@denise.theartistscloset.com>
Le vendredi 23 décembre 2011 à 11:44 -0500, John A. Sullivan III a
écrit :
> On Fri, 2011-12-23 at 17:16 +0100, Eric Dumazet wrote:
> > Le vendredi 23 décembre 2011 à 10:26 -0500, John A. Sullivan III a
> > écrit :
> >
> > > Yes. This is a problem I have with netem on ingress traffic. I use the
> > > filter on ffff: to redirect to ifb0 for the ingress traffic shaping. I
> > > cannot figure out a way to redirect a second time to ifb1 for the netem
> > > qdisc. I tried putting two action mirred statements in the filter but
> > > that did not work. Unlike eth1, I cannot attach a filter further down
> > > the ifb0 hfsc hierarchy because one can't redirect one ifb into another
> > > ifb. Thus, the only way I could figure out how to do inbound netem was
> > > to replace the terminal qdisc with netem rather than SFQ. I'd love to
> > > be able to do that differently. I tried attaching netem to the SFQ but
> > > that failed (I assume because SFQ is classless) and I tried the other
> > > way around, attaching SFQ to netem since you mentioned netem could take
> > > a class but that did not work either.
> >
> > Unfortunately, netem wants to control skbs itself, in a fifo queue.
> >
> > To implement what you want, we would need to setup a second qdisc,
> > and when packets are dequeued from internal netem fifo, queue them in
> > second qdisc.
> >
> >
> >
> I thought I tried to do that but I must have done it incorrectly. I
> would think something like:
>
> tc qdisc add dev eth1 ingress
> tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0 0 action mirred egress redirect dev ifb0
> tc qdisc add dev ifb0 root handle 4 netem delay 25ms 5ms distribution normal loss 0.1% 30%
> tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
>
> but I get:
> root@testswitch01:~# tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
> RTNETLINK answers: Operation not supported
>
> What did I do wrong? Thanks - John
>
Maybe I was not clear :
netem currently uses a fifo queue, you cant change this, without
patching kernel.
^ permalink raw reply
* Re: [patch] usb: pegasus: cleanup a couple conditions
From: Petko Manolov @ 2011-12-23 16:15 UTC (permalink / raw)
To: Dan Carpenter
Cc: Petko Manolov, Greg Kroah-Hartman, linux-usb, netdev,
kernel-janitors
In-Reply-To: <20111223104436.GA8592@elgon.mountain>
Once i thought 'loopback' may have other significant bits. Since it
doesn't look likely anymore i guess the patch is OK.
cheers,
Petko
On Fri, 23 Dec 2011, Dan Carpenter wrote:
> We recently made loopback a bool type instead of an int, so the bitwise
> AND is redundent.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 908b427..5d99b8c 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -517,7 +517,7 @@ static inline int reset_mac(pegasus_t *pegasus)
> for (i = 0; i < REG_TIMEOUT; i++) {
> get_registers(pegasus, EthCtrl1, 1, &data);
> if (~data & 0x08) {
> - if (loopback & 1)
> + if (loopback)
> break;
> if (mii_mode && (pegasus->features & HAS_HOME_PNA))
> set_register(pegasus, Gpio1, 0x34);
> @@ -561,7 +561,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
> data[1] |= 0x10; /* set 100 Mbps */
> if (mii_mode)
> data[1] = 0;
> - data[2] = (loopback & 1) ? 0x09 : 0x01;
> + data[2] = loopback ? 0x09 : 0x01;
>
> memcpy(pegasus->eth_regs, data, sizeof(data));
> ret = set_registers(pegasus, EthCtrl0, 3, data);
>
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 16:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324656963.4540.10.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
On Fri, 2011-12-23 at 17:16 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 10:26 -0500, John A. Sullivan III a
> écrit :
>
> > Yes. This is a problem I have with netem on ingress traffic. I use the
> > filter on ffff: to redirect to ifb0 for the ingress traffic shaping. I
> > cannot figure out a way to redirect a second time to ifb1 for the netem
> > qdisc. I tried putting two action mirred statements in the filter but
> > that did not work. Unlike eth1, I cannot attach a filter further down
> > the ifb0 hfsc hierarchy because one can't redirect one ifb into another
> > ifb. Thus, the only way I could figure out how to do inbound netem was
> > to replace the terminal qdisc with netem rather than SFQ. I'd love to
> > be able to do that differently. I tried attaching netem to the SFQ but
> > that failed (I assume because SFQ is classless) and I tried the other
> > way around, attaching SFQ to netem since you mentioned netem could take
> > a class but that did not work either.
>
> Unfortunately, netem wants to control skbs itself, in a fifo queue.
>
> To implement what you want, we would need to setup a second qdisc,
> and when packets are dequeued from internal netem fifo, queue them in
> second qdisc.
>
>
>
I thought I tried to do that but I must have done it incorrectly. I
would think something like:
tc qdisc add dev eth1 ingress
tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0 0 action mirred egress redirect dev ifb0
tc qdisc add dev ifb0 root handle 4 netem delay 25ms 5ms distribution normal loss 0.1% 30%
tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
but I get:
root@testswitch01:~# tc qdisc add dev ifb0 parent 4:0 handle 1: hfsc default 20
RTNETLINK answers: Operation not supported
What did I do wrong? Thanks - John
^ permalink raw reply
* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 16:16 UTC (permalink / raw)
To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324654013.10184.597.camel@denise.theartistscloset.com>
Le vendredi 23 décembre 2011 à 10:26 -0500, John A. Sullivan III a
écrit :
> Yes. This is a problem I have with netem on ingress traffic. I use the
> filter on ffff: to redirect to ifb0 for the ingress traffic shaping. I
> cannot figure out a way to redirect a second time to ifb1 for the netem
> qdisc. I tried putting two action mirred statements in the filter but
> that did not work. Unlike eth1, I cannot attach a filter further down
> the ifb0 hfsc hierarchy because one can't redirect one ifb into another
> ifb. Thus, the only way I could figure out how to do inbound netem was
> to replace the terminal qdisc with netem rather than SFQ. I'd love to
> be able to do that differently. I tried attaching netem to the SFQ but
> that failed (I assume because SFQ is classless) and I tried the other
> way around, attaching SFQ to netem since you mentioned netem could take
> a class but that did not work either.
Unfortunately, netem wants to control skbs itself, in a fifo queue.
To implement what you want, we would need to setup a second qdisc,
and when packets are dequeued from internal netem fifo, queue them in
second qdisc.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox