* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-25 11:55 UTC (permalink / raw)
To: Tore Anderson; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <4b7a2708fd4545ac65cfef6561e7f02a@greed.fud.no>
>> (*) Would it be legal for a tunnel endpoint to support ipv6 packets up
>> to 1280 bytes in size
>> but still send back a 'packet to big please use 1K mtu' message?
>
>
> I don't think this is a valid thing to do - either the tunnel server would
> forward the packet through the tunnel (fragmenting the underlaying IPv4 if
> necessary), and *not* send back a ICMPv6 PTB error at the same time, OR: it
> would need to drop the packet and *do* send back the ICMPv6 PTB. Not both
> at the same time.
>
> However, if it is going to drop the large packets and reply with the PTB, it
> will cause a blackhole with current Linux, because if it get the PTB with
> MTU=1000, it will include the frag header, but *not* reduce the actual
> packet
> size. And since this is pure IPv6 routing, the tunnel server will not be
> able
> to fragment the IPv6 packets, since IPv6 routers don't do that (the presense
> of the Fragmentation header does not change this fact).
>
> So the tunnel routers pretty much *must* support an IPv6 MTU of 1280, either
> by ensuring the outer IPv4 MTU is 1300 or larger, or by performing IPv4
> fragmentation and reassembly "under the hood".
I think you could still accept 1280 mtu packets, but for packets >= 1281 mtu
you could return packet to big, please use mtu=1000.
>
> I don't think it is worth while (or even appropriate) for the Linux IPv6
> stack
> to try to optimize for this.
Agreed. Let's ignore this.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Tore Anderson @ 2012-04-25 11:49 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGeV9Z2z3i4GYyupU2tBVbL-4nX2-h2e8MP=PP4WE2s9qw@mail.gmail.com>
* Maciej Żenczykowski
>> I think you forgot to include the explanation why. :-)
>
> I did try, I just didn't do a very good job.
Oh, because of IPv6-in-IPv4 tunnel encap overhead, I get it now.
>> I suppose. This would be invisible to IPv6, though - the
>> fragmentation and
>> reassembly happens at a lower layer than IPv6. Same as ATM for
>> example. Situation is
>> described by RFC 2460:
>
> It would be invisible (*), and you probably wouldn't really need the
> frag header in the ipv6 packet,
> but it would still be desirable to have ipv6 already have packets
> smaller than ipv4
> mtu - 20, rather than have to frag/unfrag at the tunnel endpoint.
> Since it is always more efficient to have fragmented correctly in the
> first place.
>
> (*) Would it be legal for a tunnel endpoint to support ipv6 packets
> up
> to 1280 bytes in size
> but still send back a 'packet to big please use 1K mtu' message?
I don't think this is a valid thing to do - either the tunnel server
would
forward the packet through the tunnel (fragmenting the underlaying IPv4
if
necessary), and *not* send back a ICMPv6 PTB error at the same time,
OR: it
would need to drop the packet and *do* send back the ICMPv6 PTB. Not
both
at the same time.
However, if it is going to drop the large packets and reply with the
PTB, it
will cause a blackhole with current Linux, because if it get the PTB
with
MTU=1000, it will include the frag header, but *not* reduce the actual
packet
size. And since this is pure IPv6 routing, the tunnel server will not
be able
to fragment the IPv6 packets, since IPv6 routers don't do that (the
presense
of the Fragmentation header does not change this fact).
So the tunnel routers pretty much *must* support an IPv6 MTU of 1280,
either
by ensuring the outer IPv4 MTU is 1300 or larger, or by performing IPv4
fragmentation and reassembly "under the hood".
I don't think it is worth while (or even appropriate) for the Linux
IPv6 stack
to try to optimize for this.
Tore
^ permalink raw reply
* [PATCH net-next 7/7] be2net: Fix FW download for BE
From: Padmanabh Ratnakar @ 2012-04-25 11:47 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar, Somnath Kotur
Skip flashing a FW component if that component is not present in a
particular FW UFI image.
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
drivers/net/ethernet/emulex/benet/be_hw.h | 74 ++++++++----
drivers/net/ethernet/emulex/benet/be_main.c | 168 ++++++++++++++++++---------
3 files changed, 166 insertions(+), 78 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 2673081..43167e8 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -1979,7 +1979,7 @@ int be_cmd_get_flash_crc(struct be_adapter *adapter, u8 *flashed_crc,
be_wrb_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
OPCODE_COMMON_READ_FLASHROM, sizeof(*req)+4, wrb, NULL);
- req->params.op_type = cpu_to_le32(IMG_TYPE_REDBOOT);
+ req->params.op_type = cpu_to_le32(OPTYPE_REDBOOT);
req->params.op_code = cpu_to_le32(FLASHROM_OPER_REPORT);
req->params.offset = cpu_to_le32(offset);
req->params.data_buf_size = cpu_to_le32(0x4);
diff --git a/drivers/net/ethernet/emulex/benet/be_hw.h b/drivers/net/ethernet/emulex/benet/be_hw.h
index f2c89e3..0949aa6 100644
--- a/drivers/net/ethernet/emulex/benet/be_hw.h
+++ b/drivers/net/ethernet/emulex/benet/be_hw.h
@@ -162,22 +162,23 @@
#define QUERY_FAT 1
/* Flashrom related descriptors */
+#define MAX_FLASH_COMP 32
#define IMAGE_TYPE_FIRMWARE 160
#define IMAGE_TYPE_BOOTCODE 224
#define IMAGE_TYPE_OPTIONROM 32
#define NUM_FLASHDIR_ENTRIES 32
-#define IMG_TYPE_ISCSI_ACTIVE 0
-#define IMG_TYPE_REDBOOT 1
-#define IMG_TYPE_BIOS 2
-#define IMG_TYPE_PXE_BIOS 3
-#define IMG_TYPE_FCOE_BIOS 8
-#define IMG_TYPE_ISCSI_BACKUP 9
-#define IMG_TYPE_FCOE_FW_ACTIVE 10
-#define IMG_TYPE_FCOE_FW_BACKUP 11
-#define IMG_TYPE_NCSI_FW 13
-#define IMG_TYPE_PHY_FW 99
+#define OPTYPE_ISCSI_ACTIVE 0
+#define OPTYPE_REDBOOT 1
+#define OPTYPE_BIOS 2
+#define OPTYPE_PXE_BIOS 3
+#define OPTYPE_FCOE_BIOS 8
+#define OPTYPE_ISCSI_BACKUP 9
+#define OPTYPE_FCOE_FW_ACTIVE 10
+#define OPTYPE_FCOE_FW_BACKUP 11
+#define OPTYPE_NCSI_FW 13
+#define OPTYPE_PHY_FW 99
#define TN_8022 13
#define ILLEGAL_IOCTL_REQ 2
@@ -223,6 +224,24 @@
#define FLASH_REDBOOT_START_g3 (262144)
#define FLASH_PHY_FW_START_g3 1310720
+#define IMAGE_NCSI 16
+#define IMAGE_OPTION_ROM_PXE 32
+#define IMAGE_OPTION_ROM_FCoE 33
+#define IMAGE_OPTION_ROM_ISCSI 34
+#define IMAGE_FLASHISM_JUMPVECTOR 48
+#define IMAGE_FLASH_ISM 49
+#define IMAGE_JUMP_VECTOR 50
+#define IMAGE_FIRMWARE_iSCSI 160
+#define IMAGE_FIRMWARE_COMP_iSCSI 161
+#define IMAGE_FIRMWARE_FCoE 162
+#define IMAGE_FIRMWARE_COMP_FCoE 163
+#define IMAGE_FIRMWARE_BACKUP_iSCSI 176
+#define IMAGE_FIRMWARE_BACKUP_COMP_iSCSI 177
+#define IMAGE_FIRMWARE_BACKUP_FCoE 178
+#define IMAGE_FIRMWARE_BACKUP_COMP_FCoE 179
+#define IMAGE_FIRMWARE_PHY 192
+#define IMAGE_BOOT_CODE 224
+
/************* Rx Packet Type Encoding **************/
#define BE_UNICAST_PACKET 0
#define BE_MULTICAST_PACKET 1
@@ -445,6 +464,7 @@ struct flash_comp {
unsigned long offset;
int optype;
int size;
+ int img_type;
};
struct image_hdr {
@@ -481,17 +501,19 @@ struct flash_section_hdr {
u32 format_rev;
u32 cksum;
u32 antidote;
- u32 build_no;
- u8 id_string[64];
- u32 active_entry_mask;
- u32 valid_entry_mask;
- u32 org_content_mask;
- u32 rsvd0;
- u32 rsvd1;
- u32 rsvd2;
- u32 rsvd3;
- u32 rsvd4;
-};
+ u32 num_images;
+ u8 id_string[128];
+ u32 rsvd[4];
+} __packed;
+
+struct flash_section_hdr_g2 {
+ u32 format_rev;
+ u32 cksum;
+ u32 antidote;
+ u32 build_num;
+ u8 id_string[128];
+ u32 rsvd[8];
+} __packed;
struct flash_section_entry {
u32 type;
@@ -503,10 +525,16 @@ struct flash_section_entry {
u32 rsvd0;
u32 rsvd1;
u8 ver_data[32];
-};
+} __packed;
struct flash_section_info {
u8 cookie[32];
struct flash_section_hdr fsec_hdr;
struct flash_section_entry fsec_entry[32];
-};
+} __packed;
+
+struct flash_section_info_g2 {
+ u8 cookie[32];
+ struct flash_section_hdr_g2 fsec_hdr;
+ struct flash_section_entry fsec_entry[32];
+} __packed;
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 8bc9e12..c9f757c 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -2759,6 +2759,8 @@ static void be_netpoll(struct net_device *netdev)
#endif
#define FW_FILE_HDR_SIGN "ServerEngines Corp. "
+char flash_cookie[2][16] = {"*** SE FLAS", "H DIRECTORY *** "};
+
static bool be_flash_redboot(struct be_adapter *adapter,
const u8 *p, u32 img_start, int image_size,
int hdr_size)
@@ -2792,58 +2794,101 @@ static bool phy_flashing_required(struct be_adapter *adapter)
adapter->phy.interface_type == PHY_TYPE_BASET_10GB);
}
+static bool is_comp_in_ufi(struct be_adapter *adapter,
+ struct flash_section_info *fsec, int type)
+{
+ int i = 0, img_type = 0;
+ struct flash_section_info_g2 *fsec_g2 = NULL;
+
+ if (adapter->generation != BE_GEN3)
+ fsec_g2 = (struct flash_section_info_g2 *)fsec;
+
+ for (i = 0; i < MAX_FLASH_COMP; i++) {
+ if (fsec_g2)
+ img_type = le32_to_cpu(fsec_g2->fsec_entry[i].type);
+ else
+ img_type = le32_to_cpu(fsec->fsec_entry[i].type);
+
+ if (img_type == type)
+ return true;
+ }
+ return false;
+
+}
+
+struct flash_section_info *get_fsec_info(struct be_adapter *adapter,
+ int header_size,
+ const struct firmware *fw)
+{
+ struct flash_section_info *fsec = NULL;
+ const u8 *p = fw->data;
+
+ p += header_size;
+ while (p < (fw->data + fw->size)) {
+ fsec = (struct flash_section_info *)p;
+ if (!memcmp(flash_cookie, fsec->cookie, sizeof(flash_cookie)))
+ return fsec;
+ p += 32;
+ }
+ return NULL;
+}
+
static int be_flash_data(struct be_adapter *adapter,
- const struct firmware *fw,
- struct be_dma_mem *flash_cmd, int num_of_images)
+ const struct firmware *fw,
+ struct be_dma_mem *flash_cmd,
+ int num_of_images)
{
int status = 0, i, filehdr_size = 0;
+ int img_hdrs_size = (num_of_images * sizeof(struct image_hdr));
u32 total_bytes = 0, flash_op;
int num_bytes;
const u8 *p = fw->data;
struct be_cmd_write_flashrom *req = flash_cmd->va;
const struct flash_comp *pflashcomp;
- int num_comp;
-
- static const struct flash_comp gen3_flash_types[10] = {
- { FLASH_iSCSI_PRIMARY_IMAGE_START_g3, IMG_TYPE_ISCSI_ACTIVE,
- FLASH_IMAGE_MAX_SIZE_g3},
- { FLASH_REDBOOT_START_g3, IMG_TYPE_REDBOOT,
- FLASH_REDBOOT_IMAGE_MAX_SIZE_g3},
- { FLASH_iSCSI_BIOS_START_g3, IMG_TYPE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g3},
- { FLASH_PXE_BIOS_START_g3, IMG_TYPE_PXE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g3},
- { FLASH_FCoE_BIOS_START_g3, IMG_TYPE_FCOE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g3},
- { FLASH_iSCSI_BACKUP_IMAGE_START_g3, IMG_TYPE_ISCSI_BACKUP,
- FLASH_IMAGE_MAX_SIZE_g3},
- { FLASH_FCoE_PRIMARY_IMAGE_START_g3, IMG_TYPE_FCOE_FW_ACTIVE,
- FLASH_IMAGE_MAX_SIZE_g3},
- { FLASH_FCoE_BACKUP_IMAGE_START_g3, IMG_TYPE_FCOE_FW_BACKUP,
- FLASH_IMAGE_MAX_SIZE_g3},
- { FLASH_NCSI_START_g3, IMG_TYPE_NCSI_FW,
- FLASH_NCSI_IMAGE_MAX_SIZE_g3},
- { FLASH_PHY_FW_START_g3, IMG_TYPE_PHY_FW,
- FLASH_PHY_FW_IMAGE_MAX_SIZE_g3}
+ int num_comp, hdr_size;
+ struct flash_section_info *fsec = NULL;
+
+ struct flash_comp gen3_flash_types[] = {
+ { FLASH_iSCSI_PRIMARY_IMAGE_START_g3, OPTYPE_ISCSI_ACTIVE,
+ FLASH_IMAGE_MAX_SIZE_g3, IMAGE_FIRMWARE_iSCSI},
+ { FLASH_REDBOOT_START_g3, OPTYPE_REDBOOT,
+ FLASH_REDBOOT_IMAGE_MAX_SIZE_g3, IMAGE_BOOT_CODE},
+ { FLASH_iSCSI_BIOS_START_g3, OPTYPE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g3, IMAGE_OPTION_ROM_ISCSI},
+ { FLASH_PXE_BIOS_START_g3, OPTYPE_PXE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g3, IMAGE_OPTION_ROM_PXE},
+ { FLASH_FCoE_BIOS_START_g3, OPTYPE_FCOE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g3, IMAGE_OPTION_ROM_FCoE},
+ { FLASH_iSCSI_BACKUP_IMAGE_START_g3, OPTYPE_ISCSI_BACKUP,
+ FLASH_IMAGE_MAX_SIZE_g3, IMAGE_FIRMWARE_BACKUP_iSCSI},
+ { FLASH_FCoE_PRIMARY_IMAGE_START_g3, OPTYPE_FCOE_FW_ACTIVE,
+ FLASH_IMAGE_MAX_SIZE_g3, IMAGE_FIRMWARE_FCoE},
+ { FLASH_FCoE_BACKUP_IMAGE_START_g3, OPTYPE_FCOE_FW_BACKUP,
+ FLASH_IMAGE_MAX_SIZE_g3, IMAGE_FIRMWARE_BACKUP_FCoE},
+ { FLASH_NCSI_START_g3, OPTYPE_NCSI_FW,
+ FLASH_NCSI_IMAGE_MAX_SIZE_g3, IMAGE_NCSI},
+ { FLASH_PHY_FW_START_g3, OPTYPE_PHY_FW,
+ FLASH_PHY_FW_IMAGE_MAX_SIZE_g3, IMAGE_FIRMWARE_PHY}
};
- static const struct flash_comp gen2_flash_types[8] = {
- { FLASH_iSCSI_PRIMARY_IMAGE_START_g2, IMG_TYPE_ISCSI_ACTIVE,
- FLASH_IMAGE_MAX_SIZE_g2},
- { FLASH_REDBOOT_START_g2, IMG_TYPE_REDBOOT,
- FLASH_REDBOOT_IMAGE_MAX_SIZE_g2},
- { FLASH_iSCSI_BIOS_START_g2, IMG_TYPE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g2},
- { FLASH_PXE_BIOS_START_g2, IMG_TYPE_PXE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g2},
- { FLASH_FCoE_BIOS_START_g2, IMG_TYPE_FCOE_BIOS,
- FLASH_BIOS_IMAGE_MAX_SIZE_g2},
- { FLASH_iSCSI_BACKUP_IMAGE_START_g2, IMG_TYPE_ISCSI_BACKUP,
- FLASH_IMAGE_MAX_SIZE_g2},
- { FLASH_FCoE_PRIMARY_IMAGE_START_g2, IMG_TYPE_FCOE_FW_ACTIVE,
- FLASH_IMAGE_MAX_SIZE_g2},
- { FLASH_FCoE_BACKUP_IMAGE_START_g2, IMG_TYPE_FCOE_FW_BACKUP,
- FLASH_IMAGE_MAX_SIZE_g2}
+
+ struct flash_comp gen2_flash_types[] = {
+ { FLASH_iSCSI_PRIMARY_IMAGE_START_g2, OPTYPE_ISCSI_ACTIVE,
+ FLASH_IMAGE_MAX_SIZE_g2, IMAGE_FIRMWARE_iSCSI},
+ { FLASH_REDBOOT_START_g2, OPTYPE_REDBOOT,
+ FLASH_REDBOOT_IMAGE_MAX_SIZE_g2, IMAGE_BOOT_CODE},
+ { FLASH_iSCSI_BIOS_START_g2, OPTYPE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g2, IMAGE_OPTION_ROM_ISCSI},
+ { FLASH_PXE_BIOS_START_g2, OPTYPE_PXE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g2, IMAGE_OPTION_ROM_PXE},
+ { FLASH_FCoE_BIOS_START_g2, OPTYPE_FCOE_BIOS,
+ FLASH_BIOS_IMAGE_MAX_SIZE_g2, IMAGE_OPTION_ROM_FCoE},
+ { FLASH_iSCSI_BACKUP_IMAGE_START_g2, OPTYPE_ISCSI_BACKUP,
+ FLASH_IMAGE_MAX_SIZE_g2, IMAGE_FIRMWARE_BACKUP_iSCSI},
+ { FLASH_FCoE_PRIMARY_IMAGE_START_g2, OPTYPE_FCOE_FW_ACTIVE,
+ FLASH_IMAGE_MAX_SIZE_g2, IMAGE_FIRMWARE_FCoE},
+ { FLASH_FCoE_BACKUP_IMAGE_START_g2, OPTYPE_FCOE_FW_BACKUP,
+ FLASH_IMAGE_MAX_SIZE_g2, IMAGE_FIRMWARE_BACKUP_FCoE}
};
if (adapter->generation == BE_GEN3) {
@@ -2855,22 +2900,37 @@ static int be_flash_data(struct be_adapter *adapter,
filehdr_size = sizeof(struct flash_file_hdr_g2);
num_comp = ARRAY_SIZE(gen2_flash_types);
}
+ /* Get flash section info*/
+ fsec = get_fsec_info(adapter, filehdr_size + img_hdrs_size, fw);
+ if (!fsec) {
+ dev_err(&adapter->pdev->dev,
+ "Invalid Cookie. UFI corrupted ?\n");
+ return -1;
+ }
for (i = 0; i < num_comp; i++) {
- if ((pflashcomp[i].optype == IMG_TYPE_NCSI_FW) &&
- memcmp(adapter->fw_ver, "3.102.148.0", 11) < 0)
+ if (!is_comp_in_ufi(adapter, fsec, pflashcomp[i].img_type))
continue;
- if (pflashcomp[i].optype == IMG_TYPE_PHY_FW) {
+
+ if ((pflashcomp[i].optype == OPTYPE_NCSI_FW) &&
+ memcmp(adapter->fw_ver, "3.102.148.0", 11) < 0)
+ continue;
+
+ if (pflashcomp[i].optype == OPTYPE_PHY_FW) {
if (!phy_flashing_required(adapter))
continue;
}
- if ((pflashcomp[i].optype == IMG_TYPE_REDBOOT) &&
- (!be_flash_redboot(adapter, fw->data,
- pflashcomp[i].offset, pflashcomp[i].size, filehdr_size +
- (num_of_images * sizeof(struct image_hdr)))))
+
+ hdr_size = filehdr_size +
+ (num_of_images * sizeof(struct image_hdr));
+
+ if ((pflashcomp[i].optype == OPTYPE_REDBOOT) &&
+ (!be_flash_redboot(adapter, fw->data, pflashcomp[i].offset,
+ pflashcomp[i].size, hdr_size)))
continue;
+
+ /* Flash the component */
p = fw->data;
- p += filehdr_size + pflashcomp[i].offset
- + (num_of_images * sizeof(struct image_hdr));
+ p += filehdr_size + pflashcomp[i].offset + img_hdrs_size;
if (p + pflashcomp[i].size > fw->data + fw->size)
return -1;
total_bytes = pflashcomp[i].size;
@@ -2881,12 +2941,12 @@ static int be_flash_data(struct be_adapter *adapter,
num_bytes = total_bytes;
total_bytes -= num_bytes;
if (!total_bytes) {
- if (pflashcomp[i].optype == IMG_TYPE_PHY_FW)
+ if (pflashcomp[i].optype == OPTYPE_PHY_FW)
flash_op = FLASHROM_OPER_PHY_FLASH;
else
flash_op = FLASHROM_OPER_FLASH;
} else {
- if (pflashcomp[i].optype == IMG_TYPE_PHY_FW)
+ if (pflashcomp[i].optype == OPTYPE_PHY_FW)
flash_op = FLASHROM_OPER_PHY_SAVE;
else
flash_op = FLASHROM_OPER_SAVE;
@@ -2898,7 +2958,7 @@ static int be_flash_data(struct be_adapter *adapter,
if (status) {
if ((status == ILLEGAL_IOCTL_REQ) &&
(pflashcomp[i].optype ==
- IMG_TYPE_PHY_FW))
+ OPTYPE_PHY_FW))
break;
dev_err(&adapter->pdev->dev,
"cmd to write to flash rom failed.\n");
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 6/7] be2net: Fix wrong status getting returned for MCC commands
From: Padmanabh Ratnakar @ 2012-04-25 11:47 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
MCC Response CQEs are processed as part of NAPI poll routine and
also synchronously. If MCC completionare consumed by NAPI poll
routine, wrong status is returned to synchronously waiting routine.
Fix this by getting status of MCC command from command response
instead of response CQEs.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be.h | 5 ++
drivers/net/ethernet/emulex/benet/be_cmds.c | 81 ++++++++++++++++++--------
drivers/net/ethernet/emulex/benet/be_cmds.h | 8 ++-
3 files changed, 67 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be.h b/drivers/net/ethernet/emulex/benet/be.h
index ad69cf8..4bc18ef 100644
--- a/drivers/net/ethernet/emulex/benet/be.h
+++ b/drivers/net/ethernet/emulex/benet/be.h
@@ -162,6 +162,11 @@ static inline void queue_head_inc(struct be_queue_info *q)
index_inc(&q->head, q->len);
}
+static inline void index_dec(u16 *index, u16 limit)
+{
+ *index = MODULO((*index - 1), limit);
+}
+
static inline void queue_tail_inc(struct be_queue_info *q)
{
index_inc(&q->tail, q->len);
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 4e07e58..2673081 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -61,10 +61,21 @@ static inline void be_mcc_compl_use(struct be_mcc_compl *compl)
compl->flags = 0;
}
+static struct be_cmd_resp_hdr *be_decode_resp_hdr(u32 tag0, u32 tag1)
+{
+ unsigned long addr;
+
+ addr = tag1;
+ addr = ((addr << 16) << 16) | tag0;
+ return (void *)addr;
+}
+
static int be_mcc_compl_process(struct be_adapter *adapter,
- struct be_mcc_compl *compl)
+ struct be_mcc_compl *compl)
{
u16 compl_status, extd_status;
+ struct be_cmd_resp_hdr *resp_hdr;
+ u8 opcode = 0, subsystem = 0;
/* Just swap the status to host endian; mcc tag is opaquely copied
* from mcc_wrb */
@@ -73,32 +84,36 @@ static int be_mcc_compl_process(struct be_adapter *adapter,
compl_status = (compl->status >> CQE_STATUS_COMPL_SHIFT) &
CQE_STATUS_COMPL_MASK;
- if (((compl->tag0 == OPCODE_COMMON_WRITE_FLASHROM) ||
- (compl->tag0 == OPCODE_COMMON_WRITE_OBJECT)) &&
- (compl->tag1 == CMD_SUBSYSTEM_COMMON)) {
+ resp_hdr = be_decode_resp_hdr(compl->tag0, compl->tag1);
+
+ if (resp_hdr) {
+ opcode = resp_hdr->opcode;
+ subsystem = resp_hdr->subsystem;
+ }
+
+ if (((opcode == OPCODE_COMMON_WRITE_FLASHROM) ||
+ (opcode == OPCODE_COMMON_WRITE_OBJECT)) &&
+ (subsystem == CMD_SUBSYSTEM_COMMON)) {
adapter->flash_status = compl_status;
complete(&adapter->flash_compl);
}
if (compl_status == MCC_STATUS_SUCCESS) {
- if (((compl->tag0 == OPCODE_ETH_GET_STATISTICS) ||
- (compl->tag0 == OPCODE_ETH_GET_PPORT_STATS)) &&
- (compl->tag1 == CMD_SUBSYSTEM_ETH)) {
+ if (((opcode == OPCODE_ETH_GET_STATISTICS) ||
+ (opcode == OPCODE_ETH_GET_PPORT_STATS)) &&
+ (subsystem == CMD_SUBSYSTEM_ETH)) {
be_parse_stats(adapter);
adapter->stats_cmd_sent = false;
}
- if (compl->tag0 ==
- OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES) {
- struct be_mcc_wrb *mcc_wrb =
- queue_index_node(&adapter->mcc_obj.q,
- compl->tag1);
+ if (opcode == OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES &&
+ subsystem == CMD_SUBSYSTEM_COMMON) {
struct be_cmd_resp_get_cntl_addnl_attribs *resp =
- embedded_payload(mcc_wrb);
+ (void *)resp_hdr;
adapter->drv_stats.be_on_die_temperature =
resp->on_die_temperature;
}
} else {
- if (compl->tag0 == OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES)
+ if (opcode == OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES)
be_get_temp_freq = 0;
if (compl_status == MCC_STATUS_NOT_SUPPORTED ||
@@ -108,13 +123,13 @@ static int be_mcc_compl_process(struct be_adapter *adapter,
if (compl_status == MCC_STATUS_UNAUTHORIZED_REQUEST) {
dev_warn(&adapter->pdev->dev, "This domain(VM) is not "
"permitted to execute this cmd (opcode %d)\n",
- compl->tag0);
+ opcode);
} else {
extd_status = (compl->status >> CQE_STATUS_EXTD_SHIFT) &
CQE_STATUS_EXTD_MASK;
dev_err(&adapter->pdev->dev, "Cmd (opcode %d) failed:"
"status %d, extd-status %d\n",
- compl->tag0, compl_status, extd_status);
+ opcode, compl_status, extd_status);
}
}
done:
@@ -286,7 +301,7 @@ static int be_mcc_wait_compl(struct be_adapter *adapter)
if (i == mcc_timeout) {
dev_err(&adapter->pdev->dev, "FW not responding\n");
adapter->fw_timeout = true;
- return -1;
+ return -EIO;
}
return status;
}
@@ -294,8 +309,26 @@ static int be_mcc_wait_compl(struct be_adapter *adapter)
/* Notify MCC requests and wait for completion */
static int be_mcc_notify_wait(struct be_adapter *adapter)
{
+ int status;
+ struct be_mcc_wrb *wrb;
+ struct be_mcc_obj *mcc_obj = &adapter->mcc_obj;
+ u16 index = mcc_obj->q.head;
+ struct be_cmd_resp_hdr *resp;
+
+ index_dec(&index, mcc_obj->q.len);
+ wrb = queue_index_node(&mcc_obj->q, index);
+
+ resp = be_decode_resp_hdr(wrb->tag0, wrb->tag1);
+
be_mcc_notify(adapter);
- return be_mcc_wait_compl(adapter);
+
+ status = be_mcc_wait_compl(adapter);
+ if (status == -EIO)
+ goto out;
+
+ status = resp->status;
+out:
+ return status;
}
static int be_mbox_db_ready_wait(struct be_adapter *adapter, void __iomem *db)
@@ -435,14 +468,17 @@ static void be_wrb_cmd_hdr_prepare(struct be_cmd_req_hdr *req_hdr,
struct be_mcc_wrb *wrb, struct be_dma_mem *mem)
{
struct be_sge *sge;
+ unsigned long addr = (unsigned long)req_hdr;
+ u64 req_addr = addr;
req_hdr->opcode = opcode;
req_hdr->subsystem = subsystem;
req_hdr->request_length = cpu_to_le32(cmd_len - sizeof(*req_hdr));
req_hdr->version = 0;
- wrb->tag0 = opcode;
- wrb->tag1 = subsystem;
+ wrb->tag0 = req_addr & 0xFFFFFFFF;
+ wrb->tag1 = upper_32_bits(req_addr);
+
wrb->payload_length = cmd_len;
if (mem) {
wrb->embedded |= (1 & MCC_WRB_SGE_CNT_MASK) <<
@@ -1283,13 +1319,10 @@ int be_cmd_get_die_temperature(struct be_adapter *adapter)
{
struct be_mcc_wrb *wrb;
struct be_cmd_req_get_cntl_addnl_attribs *req;
- u16 mccq_index;
int status;
spin_lock_bh(&adapter->mcc_lock);
- mccq_index = adapter->mcc_obj.q.head;
-
wrb = wrb_from_mccq(adapter);
if (!wrb) {
status = -EBUSY;
@@ -1301,8 +1334,6 @@ int be_cmd_get_die_temperature(struct be_adapter *adapter)
OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES, sizeof(*req),
wrb, NULL);
- wrb->tag1 = mccq_index;
-
be_mcc_notify(adapter);
err:
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.h b/drivers/net/ethernet/emulex/benet/be_cmds.h
index 3c54361..944f031 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.h
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.h
@@ -225,8 +225,12 @@ struct be_cmd_req_hdr {
#define RESP_HDR_INFO_OPCODE_SHIFT 0 /* bits 0 - 7 */
#define RESP_HDR_INFO_SUBSYS_SHIFT 8 /* bits 8 - 15 */
struct be_cmd_resp_hdr {
- u32 info; /* dword 0 */
- u32 status; /* dword 1 */
+ u8 opcode; /* dword 0 */
+ u8 subsystem; /* dword 0 */
+ u8 rsvd[2]; /* dword 0 */
+ u8 status; /* dword 1 */
+ u8 add_status; /* dword 1 */
+ u8 rsvd1[2]; /* dword 1 */
u32 response_length; /* dword 2 */
u32 actual_resp_len; /* dword 3 */
};
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 5/7] be2net: Fix Lancer statistics
From: Padmanabh Ratnakar @ 2012-04-25 11:46 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
Fix port num sent in command to get stats. Also skip unnecessary
parsing of stats for Lancer.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
2 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 3ba4aed..4e07e58 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -1221,7 +1221,7 @@ int lancer_cmd_get_pport_stats(struct be_adapter *adapter,
OPCODE_ETH_GET_PPORT_STATS, nonemb_cmd->size, wrb,
nonemb_cmd);
- req->cmd_params.params.pport_num = cpu_to_le16(adapter->port_num);
+ req->cmd_params.params.pport_num = cpu_to_le16(adapter->hba_port_num);
req->cmd_params.params.reset_stats = 0;
be_mcc_notify(adapter);
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 9d42fab..8bc9e12 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -421,6 +421,9 @@ void be_parse_stats(struct be_adapter *adapter)
populate_be2_stats(adapter);
}
+ if (lancer_chip(adapter))
+ goto done;
+
/* as erx_v1 is longer than v0, ok to use v1 defn for v0 access */
for_all_rx_queues(adapter, rxo, i) {
/* below erx HW counter can actually wrap around after
@@ -429,6 +432,8 @@ void be_parse_stats(struct be_adapter *adapter)
accumulate_16bit_val(&rx_stats(rxo)->rx_drops_no_frags,
(u16)erx->rx_drops_no_fragments[rxo->q.id]);
}
+done:
+ return;
}
static struct rtnl_link_stats64 *be_get_stats64(struct net_device *netdev,
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 3/7] be2net: Fix ethtool self test for Lancer
From: Padmanabh Ratnakar @ 2012-04-25 11:46 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
Lancer does not support DDR self test. Fix ethtool self test by
skipping this test for Lancer.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
index dc9f74c..0c6f06e 100644
--- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
+++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
@@ -793,7 +793,7 @@ be_self_test(struct net_device *netdev, struct ethtool_test *test, u64 *data)
}
}
- if (be_test_ddr_dma(adapter) != 0) {
+ if (!lancer_chip(adapter) && be_test_ddr_dma(adapter) != 0) {
data[3] = 1;
test->flags |= ETH_TEST_FL_FAILED;
}
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 4/7] be2net: Fix traffic stall INTx mode
From: Padmanabh Ratnakar @ 2012-04-25 11:46 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
EQ is getting armed wrongly in INTx mode as INTx interrupt is taking
some time to deassert. This can cause another interrupt while NAPI is
scheduled and scheduling a NAPI in interrupt does not take effect.
This causes interrupt to be missed and traffic stalls. Fixing this by
preventing wrong arming of EQ.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index a9a11d4..9d42fab 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -1571,7 +1571,9 @@ static int event_handle(struct be_eq_obj *eqo)
if (!num)
rearm = true;
- be_eq_notify(eqo->adapter, eqo->q.id, rearm, true, num);
+ if (num || msix_enabled(eqo->adapter))
+ be_eq_notify(eqo->adapter, eqo->q.id, rearm, true, num);
+
if (num)
napi_schedule(&eqo->napi);
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 2/7] be2net: Fix FW download in Lancer
From: Padmanabh Ratnakar @ 2012-04-25 11:46 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
Increase time given by driver to adapter for completing FW download
to 30 seconds. Also return correct status when FW download times out.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 22be08c..3ba4aed 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -1824,18 +1824,16 @@ int lancer_cmd_write_object(struct be_adapter *adapter, struct be_dma_mem *cmd,
spin_unlock_bh(&adapter->mcc_lock);
if (!wait_for_completion_timeout(&adapter->flash_compl,
- msecs_to_jiffies(12000)))
+ msecs_to_jiffies(30000)))
status = -1;
else
status = adapter->flash_status;
resp = embedded_payload(wrb);
- if (!status) {
+ if (!status)
*data_written = le32_to_cpu(resp->actual_write_len);
- } else {
+ else
*addn_status = resp->additional_status;
- status = resp->status;
- }
return status;
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 1/7] be2net: Fix VLAN/multicast packet reception
From: Padmanabh Ratnakar @ 2012-04-25 11:46 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
VLAN and multicast hardware filters are limited and can get
exhausted in adapters with many PCI functions. If setting
a VLAN or multicast filter fails due to lack of sufficient
hardware resources, these packets get dropped. Fix this by
switching to VLAN or multicast promiscous mode so that these
packets are not dropped.
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 44 ++++++++++++++++++--------
1 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index a5bc608..a9a11d4 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -797,22 +797,30 @@ static int be_vid_config(struct be_adapter *adapter, bool vf, u32 vf_num)
if (adapter->promiscuous)
return 0;
- if (adapter->vlans_added <= adapter->max_vlans) {
- /* Construct VLAN Table to give to HW */
- for (i = 0; i < VLAN_N_VID; i++) {
- if (adapter->vlan_tag[i]) {
- vtag[ntags] = cpu_to_le16(i);
- ntags++;
- }
- }
- status = be_cmd_vlan_config(adapter, adapter->if_handle,
- vtag, ntags, 1, 0);
- } else {
- status = be_cmd_vlan_config(adapter, adapter->if_handle,
- NULL, 0, 1, 1);
+ if (adapter->vlans_added > adapter->max_vlans)
+ goto set_vlan_promisc;
+
+ /* Construct VLAN Table to give to HW */
+ for (i = 0; i < VLAN_N_VID; i++)
+ if (adapter->vlan_tag[i])
+ vtag[ntags++] = cpu_to_le16(i);
+
+ status = be_cmd_vlan_config(adapter, adapter->if_handle,
+ vtag, ntags, 1, 0);
+
+ /* Set to VLAN promisc mode as setting VLAN filter failed */
+ if (status) {
+ dev_info(&adapter->pdev->dev, "Exhausted VLAN HW filters.\n");
+ dev_info(&adapter->pdev->dev, "Disabling HW VLAN filtering.\n");
+ goto set_vlan_promisc;
}
return status;
+
+set_vlan_promisc:
+ status = be_cmd_vlan_config(adapter, adapter->if_handle,
+ NULL, 0, 1, 1);
+ return status;
}
static int be_vlan_add_vid(struct net_device *netdev, u16 vid)
@@ -862,6 +870,7 @@ ret:
static void be_set_rx_mode(struct net_device *netdev)
{
struct be_adapter *adapter = netdev_priv(netdev);
+ int status;
if (netdev->flags & IFF_PROMISC) {
be_cmd_rx_filter(adapter, IFF_PROMISC, ON);
@@ -908,7 +917,14 @@ static void be_set_rx_mode(struct net_device *netdev)
}
}
- be_cmd_rx_filter(adapter, IFF_MULTICAST, ON);
+ status = be_cmd_rx_filter(adapter, IFF_MULTICAST, ON);
+
+ /* Set to MCAST promisc mode if setting MULTICAST address fails */
+ if (status) {
+ dev_info(&adapter->pdev->dev, "Exhausted multicast HW filters.\n");
+ dev_info(&adapter->pdev->dev, "Disabling HW multicast filtering.\n");
+ be_cmd_rx_filter(adapter, IFF_ALLMULTI, ON);
+ }
done:
return;
}
--
1.6.0.2
^ permalink raw reply related
* [PATCH net-next 0/7] be2net fixes
From: Padmanabh Ratnakar @ 2012-04-25 11:45 UTC (permalink / raw)
To: netdev; +Cc: Padmanabh Ratnakar
Please apply.
Thanks,
Padmanabh
Padmanabh Ratnakar (7):
be2net: Fix VLAN/multicast packet reception
be2net: Fix FW download in Lancer
be2net: Fix ethtool self test for Lancer
be2net: Fix traffic stall INTx mode
be2net: Fix Lancer statistics
be2net: Fix wrong status getting returned for MCC commands
be2net: Fix FW download for BE
drivers/net/ethernet/emulex/benet/be.h | 5 +
drivers/net/ethernet/emulex/benet/be_cmds.c | 93 +++++++----
drivers/net/ethernet/emulex/benet/be_cmds.h | 8 +-
drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
drivers/net/ethernet/emulex/benet/be_hw.h | 74 ++++++---
drivers/net/ethernet/emulex/benet/be_main.c | 221 ++++++++++++++++--------
6 files changed, 276 insertions(+), 127 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-25 11:02 UTC (permalink / raw)
To: Tore Anderson; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <7333a1d306fa2eca215bc9f55d23d03c@greed.fud.no>
> I think you forgot to include the explanation why. :-)
I did try, I just didn't do a very good job.
> I suppose. This would be invisible to IPv6, though - the fragmentation and
> reassembly happens at a lower layer than IPv6. Same as ATM for example. Situation is
> described by RFC 2460:
It would be invisible (*), and you probably wouldn't really need the
frag header in the ipv6 packet,
but it would still be desirable to have ipv6 already have packets
smaller than ipv4
mtu - 20, rather than have to frag/unfrag at the tunnel endpoint.
Since it is always more efficient to have fragmented correctly in the
first place.
(*) Would it be legal for a tunnel endpoint to support ipv6 packets up
to 1280 bytes in size
but still send back a 'packet to big please use 1K mtu' message?
> «On any link that cannot convey a 1280-octet packet in one piece,
> link-specific fragmentation and reassembly must be provided at a layer below IPv6.»
True... I wonder how far we should bend over, just because it'll do
the work for us,
doesn't mean it isn't more efficient to do it ourselves...
Eh, not sure it's really worth the bother, I've never seen ipv6
tunneled over something with a small (<1280) but not tiny (>200) mtu.
>> (re: Eric's patch, I think it should protect itself against malicious
>> PMTU messages with too small MTUs, like 0 or 1 or 68 [not enough for
>> timestamped ipv6/tcp)
>
>
> Does this happen for IPv4, I wonder? IMHO, it makes sense to keep the the
> minimum
> PMTUDs allowed in sync. If PMTUD=1 is allowed in IPv4, and this is not
> problematic,
> I don't see why it couldn't be allowed in IPv6 either.
>
> Tore
>
>
--
Maciej A. Żenczykowski
Kernel Networking Developer @ Google
1600 Amphitheatre Parkway, Mountain View, CA 94043
tel: +1 (650) 253-0062
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Tore Anderson @ 2012-04-25 10:45 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGf5oTd9mQsXqKj3uXgweMwjs9N3VSeXBHjDXQ7vG0NaSg@mail.gmail.com>
* Maciej Żenczykowski
>> The sensible default would be either 1280 (and keep allfrag
>> feature), or
>> the minimum IPv4 PMTU currently enforced by the kernel + 20 bytes
>> (to
>> compensate for the larger IPv6 header size). I don't know what the
>> current
>> minimum PMTU is.
>
> I'd actually go with min IPv4 PMTU - 20, not + 20, see below for why.
I think you forgot to include the explanation why. :-)
> Hmm, it may be best to honour PMTUs below 1280 to some small but not
> too small value (512?), and below that give up, say mtu remains 1280
> but still add the frag header.
That is also possible, yes.
> Hmm, I thought one way to implement an IPv6 over IPv4 tunnel was to
> basically
> rely on IPv4 fragmentation, hence you would actually be using the
> same mechanism
> as for an IPv6-IPv4 translator, then you would be tunneling the IPv6
> packet over IPv4,
> so your IPv6 mtu would be 20 less than the v4 one, not 20 more which
> you get if you
> replace the v6 header with a smaller v4 one.
>
> Anyway, this certainly seems like an ipv6-in-ipv4 tunneling mechanism
> which would currently work, wouldn't it?
I suppose. This would be invisible to IPv6, though - the fragmentation
and reassembly
happens at a lower layer than IPv6. Same as ATM for example. Situation
is described
by RFC 2460:
«On any link that cannot convey a 1280-octet packet in one piece,
link-specific
fragmentation and reassembly must be provided at a layer below IPv6.»
> (re: Eric's patch, I think it should protect itself against malicious
> PMTU messages with too small MTUs, like 0 or 1 or 68 [not enough for
> timestamped ipv6/tcp)
Does this happen for IPv4, I wonder? IMHO, it makes sense to keep the
the minimum
PMTUDs allowed in sync. If PMTUD=1 is allowed in IPv4, and this is not
problematic,
I don't see why it couldn't be allowed in IPv6 either.
Tore
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-25 10:44 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGf5oTd9mQsXqKj3uXgweMwjs9N3VSeXBHjDXQ7vG0NaSg@mail.gmail.com>
On Wed, 2012-04-25 at 03:30 -0700, Maciej Żenczykowski wrote:
> Hmm, it may be best to honour PMTUs below 1280 to some small but not
> too small value (512?), and below that give up, say mtu remains 1280
> but still add the frag header.
>
Thats a good idea.
> (re: Eric's patch, I think it should protect itself against malicious
> PMTU messages with too small MTUs, like 0 or 1 or 68 [not enough for
> timestamped ipv6/tcp)
Yes, in fact we are going to keep the allfrag feature, and arm it if we
receive an mtu < 512.
I'll send a v3 patch soon.
Thanks
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-25 10:30 UTC (permalink / raw)
To: Tore Anderson; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <4c435f101fb7c653fd3b4e81980250e7@greed.fud.no>
> The sensible default would be either 1280 (and keep allfrag feature), or
> the minimum IPv4 PMTU currently enforced by the kernel + 20 bytes (to
> compensate for the larger IPv6 header size). I don't know what the current
> minimum PMTU is.
I'd actually go with min IPv4 PMTU - 20, not + 20, see below for why.
Hmm, it may be best to honour PMTUs below 1280 to some small but not
too small value (512?), and below that give up, say mtu remains 1280
but still add the frag header.
> Also, I'm not really sure if the IPv4 minimum PMTU is defined as 576 or
> 68 bytes. There are some conflicting information out there, and nothing
> really
> authoritative either way (that I've found at least).
Yeah, I've seen conflicting information.
I wonder if there's some tiny embedded devices out there with
miniscule mtus, because of tiny amounts of ram.
> Yup, only difference is that an IPv6 tunnel is guaranteed to have a MTU of
> 1280, so no need for allfrag or dropping PMTU below 1280. No such guarantees
> exist for IPv4 links.
Hmm, I thought one way to implement an IPv6 over IPv4 tunnel was to basically
rely on IPv4 fragmentation, hence you would actually be using the same mechanism
as for an IPv6-IPv4 translator, then you would be tunneling the IPv6
packet over IPv4,
so your IPv6 mtu would be 20 less than the v4 one, not 20 more which
you get if you
replace the v6 header with a smaller v4 one.
Anyway, this certainly seems like an ipv6-in-ipv4 tunneling mechanism
which would currently work, wouldn't it?
(re: Eric's patch, I think it should protect itself against malicious
PMTU messages with too small MTUs, like 0 or 1 or 68 [not enough for
timestamped ipv6/tcp)
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-25 10:15 UTC (permalink / raw)
To: Tore Anderson; +Cc: Maciej Żenczykowski, David Miller, netdev, Tom Herbert
In-Reply-To: <4c435f101fb7c653fd3b4e81980250e7@greed.fud.no>
On Wed, 2012-04-25 at 12:04 +0200, Tore Anderson wrote:
>
> Also, I'm not really sure if the IPv4 minimum PMTU is defined as 576 or
> 68 bytes. There are some conflicting information out there, and nothing
> really
> authoritative either way (that I've found at least).
576 certainly not.
68 seems to be allowed
^ permalink raw reply
* Re: My e1000e GBE card is eating all port 623 pks
From: Joakim Tjernlund @ 2012-04-25 10:13 UTC (permalink / raw)
To: Brice Goglin; +Cc: Jesse Brandeburg, netdev
In-Reply-To: <4F97CA9B.3060105@ens-lyon.org>
Brice Goglin <Brice.Goglin@ens-lyon.org> wrote on 2012/04/25 11:57:47:
>
> On 25/04/2012 11:06, Joakim Tjernlund wrote:
> > Jesse Brandeburg<jesse.brandeburg@intel.com> wrote on 2012/04/24 23:51:28:
> >> On Sat, 21 Apr 2012 15:31:09 +0200
> >> Joakim Tjernlund<joakim.tjernlund@transmode.se> wrote:
> >>
> >>> Looks like port 623 is some mgmt protocol and our e1000e boards are eating these
> >>> pkgs and this trips NIS, finger and yptest hangs for a long time before timing out and
> >>> moving on.
> >>>
> >>> Is there a way to tell the network stack not to eat port 623 pkgs or
> >>> have NIS not to use port 623?
> >> I think you might be looking for something like portreserve
> >> (see man portreserve)
> > Seen portreserve on the web(gentoo does not have it) but it seems like a workaround.
> > Should not eating port 623 be something one should turn on? Now it is default on
> > and I cannot find a way to turn it off.
> >
>
> It might be related to IPMI (UDP/623 iirc). Try looking in your BIOS
> and/or network firmware config at boot, there might be things to disable
> there.
Been looking around but didn't find anything to turn this off, can you be more specific
where to look?
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Tore Anderson @ 2012-04-25 10:04 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGcGH6Su8Lvt34eTDF2iv2HSipo2DvybXGQA4pXZj_qb9Q@mail.gmail.com>
* Maciej Żenczykowski
>> That is a different issue entirely, but I don't disagree with you. A
>> "min_pmtu" sysctl or something like that would be useful.
>
> I don't really know what the default value should be? Something
> around 500?
> [to handle IPv4s min mtu of 576?]
The sensible default would be either 1280 (and keep allfrag feature),
or
the minimum IPv4 PMTU currently enforced by the kernel + 20 bytes (to
compensate for the larger IPv6 header size). I don't know what the
current
minimum PMTU is.
Also, I'm not really sure if the IPv4 minimum PMTU is defined as 576 or
68 bytes. There are some conflicting information out there, and nothing
really
authoritative either way (that I've found at least).
> Do we have any idea what values of small mtu actually show up in
> practice?
I have no data on this, I'm afraid. But I believe small MTUs (<1260,
which
currently triggers the need for allfrag), are very rare - at least
where I'm
from. Anectdotal, but - we've been running our corporate web site
IPv6-only
with IPv4 access through stateless translation on a Linux server with
the
buggy allfrag feature for several months, and there has been no
complaints.
>> However, the use case for the allfrag feature is not handling
>> tunnels,
>> but IPv4<->IPv6 translation. The issue is that a IPv6 host may very
>> well
>> receive an ICMPv6 Packet Too Big indicating a PMTU of <1280 that was
>> originally transmitted by an IPv4 router (as an ICMPv4 Need To
>> Fragment)
>> and underwent translation to IPv6.
>
> Very good point, although that's basically kind of like half a tunnel
> ;-)
Yup, only difference is that an IPv6 tunnel is guaranteed to have a MTU
of
1280, so no need for allfrag or dropping PMTU below 1280. No such
guarantees
exist for IPv4 links.
Tore
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-25 10:02 UTC (permalink / raw)
To: Tore Anderson; +Cc: Maciej Żenczykowski, David Miller, netdev, Tom Herbert
In-Reply-To: <1335346710.3274.24.camel@edumazet-glaptop>
On Wed, 2012-04-25 at 11:38 +0200, Eric Dumazet wrote:
> Hmm, but what if we change linux to choice a) instead of b) ?
>
> That is, not cap mtu to minimum value 1280 (and not use anymore
> RTAX_FEATURE_ALLFRAG) : dst_allfrag() would be always false.
>
> In this case, do we still need to send the frag header ?
>
> I ask this because some TSO6 implementations probably dont cope very
> well with this added header (untested path)
>
So a patch against net-next would looks like :
(Incredible, we remove some code in linux ;) )
include/linux/rtnetlink.h | 2 +-
include/net/dst.h | 7 -------
include/net/inet_sock.h | 2 +-
net/ipv6/ip6_output.c | 14 +++++---------
net/ipv6/route.c | 28 ----------------------------
net/ipv6/xfrm6_output.c | 9 ++++-----
6 files changed, 11 insertions(+), 51 deletions(-)
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 2c1de89..92ed273 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -380,7 +380,7 @@ enum {
#define RTAX_FEATURE_ECN 0x00000001
#define RTAX_FEATURE_SACK 0x00000002
#define RTAX_FEATURE_TIMESTAMP 0x00000004
-#define RTAX_FEATURE_ALLFRAG 0x00000008
+#define RTAX_FEATURE_ALLFRAG 0x00000008 /* not used anymore */
struct rta_session {
__u8 proto;
diff --git a/include/net/dst.h b/include/net/dst.h
index ff4da42..e69c55b 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -226,13 +226,6 @@ static inline void set_dst_metric_rtt(struct dst_entry *dst, int metric,
dst_metric_set(dst, metric, jiffies_to_msecs(rtt));
}
-static inline u32
-dst_allfrag(const struct dst_entry *dst)
-{
- int ret = dst_feature(dst, RTAX_FEATURE_ALLFRAG);
- return ret;
-}
-
static inline int
dst_metric_locked(const struct dst_entry *dst, int metric)
{
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index ae17e13..381ddc3 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -177,7 +177,7 @@ struct inet_sock {
};
#define IPCORK_OPT 1 /* ip-options has been held in ipcork.opt */
-#define IPCORK_ALLFRAG 2 /* always fragment (for ipv6 for now) */
+#define IPCORK_ALLFRAG 2 /* always fragment (for ipv6 for now), unused */
static inline struct inet_sock *inet_sk(const struct sock *sk)
{
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index b7ca461..314275e 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -152,11 +152,10 @@ static int ip6_finish_output2(struct sk_buff *skb)
static int ip6_finish_output(struct sk_buff *skb)
{
- if ((skb->len > ip6_skb_dst_mtu(skb) && !skb_is_gso(skb)) ||
- dst_allfrag(skb_dst(skb)))
+ if (skb->len > ip6_skb_dst_mtu(skb) && !skb_is_gso(skb))
return ip6_fragment(skb, ip6_finish_output2);
- else
- return ip6_finish_output2(skb);
+
+ return ip6_finish_output2(skb);
}
int ip6_output(struct sk_buff *skb)
@@ -1255,8 +1254,6 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
mtu = np->frag_size;
}
cork->fragsize = mtu;
- if (dst_allfrag(rt->dst.path))
- cork->flags |= IPCORK_ALLFRAG;
cork->length = 0;
sk->sk_sndmsg_page = NULL;
sk->sk_sndmsg_off = 0;
@@ -1335,7 +1332,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
while (length > 0) {
/* Check if the remaining data fits into current packet. */
- copy = (cork->length <= mtu && !(cork->flags & IPCORK_ALLFRAG) ? mtu : maxfraglen) - skb->len;
+ copy = (cork->length <= mtu ? mtu : maxfraglen) - skb->len;
if (copy < length)
copy = maxfraglen - skb->len;
@@ -1360,7 +1357,7 @@ alloc_new_skb:
* we know we need more fragment(s).
*/
datalen = length + fraggap;
- if (datalen > (cork->length <= mtu && !(cork->flags & IPCORK_ALLFRAG) ? mtu : maxfraglen) - fragheaderlen)
+ if (datalen > (cork->length <= mtu ? mtu : maxfraglen) - fragheaderlen)
datalen = maxfraglen - fragheaderlen;
fraglen = datalen + fragheaderlen;
@@ -1550,7 +1547,6 @@ static void ip6_cork_release(struct inet_sock *inet, struct ipv6_pinfo *np)
if (inet->cork.base.dst) {
dst_release(inet->cork.base.dst);
inet->cork.base.dst = NULL;
- inet->cork.base.flags &= ~IPCORK_ALLFRAG;
}
memset(&inet->cork.fl, 0, sizeof(inet->cork.fl));
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0aefc36..43e2ec1 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1044,12 +1044,6 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
if (mtu < dst_mtu(dst) && rt6->rt6i_dst.plen == 128) {
rt6->rt6i_flags |= RTF_MODIFIED;
- if (mtu < IPV6_MIN_MTU) {
- u32 features = dst_metric(dst, RTAX_FEATURES);
- mtu = IPV6_MIN_MTU;
- features |= RTAX_FEATURE_ALLFRAG;
- dst_metric_set(dst, RTAX_FEATURES, features);
- }
dst_metric_set(dst, RTAX_MTU, mtu);
}
}
@@ -1707,7 +1701,6 @@ static void rt6_do_pmtu_disc(const struct in6_addr *daddr, const struct in6_addr
struct net *net, u32 pmtu, int ifindex)
{
struct rt6_info *rt, *nrt;
- int allfrag = 0;
again:
rt = rt6_lookup(net, daddr, saddr, ifindex, 0);
if (!rt)
@@ -1721,17 +1714,6 @@ again:
if (pmtu >= dst_mtu(&rt->dst))
goto out;
- if (pmtu < IPV6_MIN_MTU) {
- /*
- * According to RFC2460, PMTU is set to the IPv6 Minimum Link
- * MTU (1280) and a fragment header should always be included
- * after a node receiving Too Big message reporting PMTU is
- * less than the IPv6 Minimum Link MTU.
- */
- pmtu = IPV6_MIN_MTU;
- allfrag = 1;
- }
-
/* New mtu received -> path was valid.
They are sent only in response to data packets,
so that this nexthop apparently is reachable. --ANK
@@ -1745,11 +1727,6 @@ again:
*/
if (rt->rt6i_flags & RTF_CACHE) {
dst_metric_set(&rt->dst, RTAX_MTU, pmtu);
- if (allfrag) {
- u32 features = dst_metric(&rt->dst, RTAX_FEATURES);
- features |= RTAX_FEATURE_ALLFRAG;
- dst_metric_set(&rt->dst, RTAX_FEATURES, features);
- }
rt6_update_expires(rt, net->ipv6.sysctl.ip6_rt_mtu_expires);
rt->rt6i_flags |= RTF_MODIFIED;
goto out;
@@ -1767,11 +1744,6 @@ again:
if (nrt) {
dst_metric_set(&nrt->dst, RTAX_MTU, pmtu);
- if (allfrag) {
- u32 features = dst_metric(&nrt->dst, RTAX_FEATURES);
- features |= RTAX_FEATURE_ALLFRAG;
- dst_metric_set(&nrt->dst, RTAX_FEATURES, features);
- }
/* According to RFC 1981, detecting PMTU increase shouldn't be
* happened within 5 mins, the recommended timer is 10 mins.
diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 8755a30..8242af0 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -146,11 +146,10 @@ static int __xfrm6_output(struct sk_buff *skb)
return -EMSGSIZE;
}
- if (x->props.mode == XFRM_MODE_TUNNEL &&
- ((skb->len > mtu && !skb_is_gso(skb)) ||
- dst_allfrag(skb_dst(skb)))) {
- return ip6_fragment(skb, x->outer_mode->afinfo->output_finish);
- }
+ if (x->props.mode == XFRM_MODE_TUNNEL && skb->len > mtu &&
+ !skb_is_gso(skb))
+ return ip6_fragment(skb, x->outer_mode->afinfo->output_finish);
+
return x->outer_mode->afinfo->output_finish(skb);
}
^ permalink raw reply related
* Re: My e1000e GBE card is eating all port 623 pks
From: Brice Goglin @ 2012-04-25 9:57 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: Jesse Brandeburg, netdev
In-Reply-To: <OF497D4E52.CC9EA3C0-ONC12579EB.0031DCB3-C12579EB.003213E4@transmode.se>
On 25/04/2012 11:06, Joakim Tjernlund wrote:
> Jesse Brandeburg<jesse.brandeburg@intel.com> wrote on 2012/04/24 23:51:28:
>> On Sat, 21 Apr 2012 15:31:09 +0200
>> Joakim Tjernlund<joakim.tjernlund@transmode.se> wrote:
>>
>>> Looks like port 623 is some mgmt protocol and our e1000e boards are eating these
>>> pkgs and this trips NIS, finger and yptest hangs for a long time before timing out and
>>> moving on.
>>>
>>> Is there a way to tell the network stack not to eat port 623 pkgs or
>>> have NIS not to use port 623?
>> I think you might be looking for something like portreserve
>> (see man portreserve)
> Seen portreserve on the web(gentoo does not have it) but it seems like a workaround.
> Should not eating port 623 be something one should turn on? Now it is default on
> and I cannot find a way to turn it off.
>
It might be related to IPMI (UDP/623 iirc). Try looking in your BIOS
and/or network firmware config at boot, there might be things to disable
there.
Brice
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-25 9:52 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Tore Anderson, David Miller, netdev, Tom Herbert
In-Reply-To: <1335346710.3274.24.camel@edumazet-glaptop>
> Hmm, but what if we change linux to choice a) instead of b) ?
>
> That is, not cap mtu to minimum value 1280 (and not use anymore
> RTAX_FEATURE_ALLFRAG) : dst_allfrag() would be always false.
>
> In this case, do we still need to send the frag header ?
Yeah, I was wondering about that myself.
By my reading of the relevant RFC it's not quite clear whether you
truly must include the frag header even if you choose to obey the
lower than 1280 mtu.
Although I don't see any reason why you would need to...
So long as there's a decent minimum pmtu we're willing to obey.
> I ask this because some TSO6 implementations probably dont cope very
> well with this added header (untested path)
Yes, I was thinking the same thing, hence why I mentioned GSO.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Tore Anderson @ 2012-04-25 9:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Maciej Żenczykowski, David Miller, netdev, Tom Herbert
In-Reply-To: <1335346710.3274.24.camel@edumazet-glaptop>
* Eric Dumazet
> Hmm, but what if we change linux to choice a) instead of b) ?
>
> That is, not cap mtu to minimum value 1280 (and not use anymore
> RTAX_FEATURE_ALLFRAG) : dst_allfrag() would be always false.
Yep.
> In this case, do we still need to send the frag header ?
No - the (translated) IPv4 packet should then fit onto the small-MTU
IPv4 link without any fragmentation (by the IPv4 router) required. So
you end up with perfectly regular end-to-end Path MTU Discovery.
Tore
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Maciej Żenczykowski @ 2012-04-25 9:48 UTC (permalink / raw)
To: Tore Anderson; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <f88609a3e80bbe53233e62dec2699a3e@greed.fud.no>
>> I get that we _choose_ to behave such, and I agree this adheres to
>> specs.
>
> "Chose" (past), not "choose" (present). ;-)
Details, but until we 'choose' to change it we continuously 'choose'
to have the current behaviour. ;-)
> This patch does not make this choice. This patch merely fixes a bug in
> the implementation of the choice that was made a long time ago.
yes, I wasn't saying this patch was bad, I was just wanting to point
that we should perhaps revisit the design choice.
>> But I'm not convinced that (even though this is allowed per RFC) this
>> is the right choice.
>
> That is a different issue entirely, but I don't disagree with you. A
> "min_pmtu" sysctl or something like that would be useful.
I don't really know what the default value should be? Something around 500?
[to handle IPv4s min mtu of 576?]
Do we have any idea what values of small mtu actually show up in practice?
> Actually, in IPv6, fragmentation *must* be performed by end hosts,
> routers (including tunnel end points) *cannot* fragment.
Yes, I miss-phrased that, that's what I meant.
> However, the use case for the allfrag feature is not handling tunnels,
> but IPv4<->IPv6 translation. The issue is that a IPv6 host may very well
> receive an ICMPv6 Packet Too Big indicating a PMTU of <1280 that was
> originally transmitted by an IPv4 router (as an ICMPv4 Need To Fragment)
> and underwent translation to IPv6.
Very good point, although that's basically kind of like half a tunnel ;-)
> In case you're interested, I have a slide deck below that explains the
> use case for IPv4<->IPv6 translation. Slide 25 is about the particular
> corner case where the allfrag feature is necessary. URL:
>
> http://fud.no/talks/20120417-RIPE64-The_Case_for_IPv6_Only_Data_Centres.pdf
Yes, I've seen this slide set a couple days ago - very good.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Tore Anderson @ 2012-04-25 9:20 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Eric Dumazet, David Miller, netdev, Tom Herbert
In-Reply-To: <CANP3RGdhs8s_RytR=f8ismSZdGs91bpVq=ZAjb0EOm-gCsDPAw@mail.gmail.com>
* Maciej Żenczykowski
>> But we chose to _not_ decrease mtu and adhere to the specs.
>
> I get that we _choose_ to behave such, and I agree this adheres to
> specs.
"Chose" (past), not "choose" (present). ;-)
This patch does not make this choice. This patch merely fixes a bug in
the implementation of the choice that was made a long time ago.
> But I'm not convinced that (even though this is allowed per RFC) this
> is the right choice.
That is a different issue entirely, but I don't disagree with you. A
"min_pmtu" sysctl or something like that would be useful.
> Also note that IPv6 prefers to see fragmentation happen at the end
> hosts, and not at the routers.
> Although of course it doesn't treat a tunnel end point as a router.
Actually, in IPv6, fragmentation *must* be performed by end hosts,
routers (including tunnel end points) *cannot* fragment.
However, the use case for the allfrag feature is not handling tunnels,
but IPv4<->IPv6 translation. The issue is that a IPv6 host may very
well
receive an ICMPv6 Packet Too Big indicating a PMTU of <1280 that was
originally transmitted by an IPv4 router (as an ICMPv4 Need To
Fragment)
and underwent translation to IPv6.
In this case, the IPv6 node does not need to reduce the PMTU to <1280
(Linux does not), but it is not invalid to have a <1280 MTU link in the
IPv4 internet either, so something else must be done for the
communication to work. The solution is then to include the IPv6
Fragment
extension header, so that the translator have a suitable Identification
value to copy into the translated IPv4 header, and may therefore clear
the Don't Fragment flag, so that the IPv4 router will fragment the
packet as it is forwarded onto the low-MTU link.
In case you're interested, I have a slide deck below that explains the
use case for IPv4<->IPv6 translation. Slide 25 is about the particular
corner case where the allfrag feature is necessary. URL:
http://fud.no/talks/20120417-RIPE64-The_Case_for_IPv6_Only_Data_Centres.pdf
Tore
^ permalink raw reply
* Re: [PATCH net-next] ipv6: RTAX_FEATURE_ALLFRAG causes inefficient TCP segment sizing
From: Eric Dumazet @ 2012-04-25 9:38 UTC (permalink / raw)
To: Tore Anderson; +Cc: Maciej Żenczykowski, David Miller, netdev, Tom Herbert
In-Reply-To: <f88609a3e80bbe53233e62dec2699a3e@greed.fud.no>
On Wed, 2012-04-25 at 11:20 +0200, Tore Anderson wrote:
> * Maciej Żenczykowski
>
> >> But we chose to _not_ decrease mtu and adhere to the specs.
> >
> > I get that we _choose_ to behave such, and I agree this adheres to
> > specs.
>
> "Chose" (past), not "choose" (present). ;-)
>
> This patch does not make this choice. This patch merely fixes a bug in
> the implementation of the choice that was made a long time ago.
>
> > But I'm not convinced that (even though this is allowed per RFC) this
> > is the right choice.
>
> That is a different issue entirely, but I don't disagree with you. A
> "min_pmtu" sysctl or something like that would be useful.
>
> > Also note that IPv6 prefers to see fragmentation happen at the end
> > hosts, and not at the routers.
> > Although of course it doesn't treat a tunnel end point as a router.
>
> Actually, in IPv6, fragmentation *must* be performed by end hosts,
> routers (including tunnel end points) *cannot* fragment.
>
> However, the use case for the allfrag feature is not handling tunnels,
> but IPv4<->IPv6 translation. The issue is that a IPv6 host may very
> well
> receive an ICMPv6 Packet Too Big indicating a PMTU of <1280 that was
> originally transmitted by an IPv4 router (as an ICMPv4 Need To
> Fragment)
> and underwent translation to IPv6.
>
> In this case, the IPv6 node does not need to reduce the PMTU to <1280
> (Linux does not), but it is not invalid to have a <1280 MTU link in the
> IPv4 internet either, so something else must be done for the
> communication to work. The solution is then to include the IPv6
> Fragment
> extension header, so that the translator have a suitable Identification
> value to copy into the translated IPv4 header, and may therefore clear
> the Don't Fragment flag, so that the IPv4 router will fragment the
> packet as it is forwarded onto the low-MTU link.
>
> In case you're interested, I have a slide deck below that explains the
> use case for IPv4<->IPv6 translation. Slide 25 is about the particular
> corner case where the allfrag feature is necessary. URL:
>
> http://fud.no/talks/20120417-RIPE64-The_Case_for_IPv6_Only_Data_Centres.pdf
Hmm, but what if we change linux to choice a) instead of b) ?
That is, not cap mtu to minimum value 1280 (and not use anymore
RTAX_FEATURE_ALLFRAG) : dst_allfrag() would be always false.
In this case, do we still need to send the frag header ?
I ask this because some TSO6 implementations probably dont cope very
well with this added header (untested path)
^ permalink raw reply
* Re: [PATCH 1/1] ipvs: kernel oops - do_ip_vs_get_ctl
From: Hans Schillstrom @ 2012-04-25 9:36 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: horms@verge.net.au, ja@ssi.bg, wensong@linux-vs.org,
lvs-devel@vger.kernel.org, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org, hans@schillstrom.com
In-Reply-To: <1335344761.6337.42.camel@t520>
On Wednesday 25 April 2012 11:06:01 Jesper Dangaard Brouer wrote:
> Hi Hans,
>
> Thank you for your work.
> Just some whitespace nitpicks below.
OK thanks I forgott to run checkpatch ...
I'll send a new patch
> I have not been able to reproduce the bug in:
> https://bugzilla.redhat.com/show_bug.cgi?id=806704
>
> Can you recommend a (better) way to reproduce this bug?
> I just want to be able to verify that this fixes the bug in question.
I think you need two threads or procs.
Start a modprobe in thread one then issue a ipvs ioctl in the other.
The timing is not that easy here :-)
>
>
> On Wed, 2012-04-25 at 09:44 +0200, Hans Schillstrom wrote:
> > Change order of init so netns init is ready
> > when register ioctl and netlink.
> >
> > Reported-by: "Ryan O'Hara" <rohara@redhat.com>
> > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> > ---
> > include/net/ip_vs.h | 2 +
> > net/netfilter/ipvs/ip_vs_core.c | 9 ++++++
> > net/netfilter/ipvs/ip_vs_ctl.c | 52 ++++++++++++++++++++++----------------
> > 3 files changed, 41 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > index f967395..93b81aa 100644
> > --- a/include/net/ip_vs.h
> > +++ b/include/net/ip_vs.h
> > @@ -1201,6 +1201,8 @@ ip_vs_lookup_real_service(struct net *net, int af, __u16 protocol,
> >
> > extern int ip_vs_use_count_inc(void);
> > extern void ip_vs_use_count_dec(void);
> > +extern int ip_vs_register_nl_ioctl(void);
> > +extern void ip_vs_unregister_nl_ioctl(void);
> > extern int ip_vs_control_init(void);
> > extern void ip_vs_control_cleanup(void);
> > extern struct ip_vs_dest *
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index d8b1d30..c8f36b9 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1995,10 +1995,18 @@ static int __init ip_vs_init(void)
> > goto cleanup_dev;
> > }
> >
> > + ret = ip_vs_register_nl_ioctl();
> > + if (ret < 0) {
> > + pr_err("can't register netlink/ioctl.\n");
> > + goto cleanup_hooks;
> > + }
> > +
> > pr_info("ipvs loaded.\n");
> >
> > return ret;
> >
> > +cleanup_hooks:
> > + nf_unregister_hooks(ip_vs_ops, ARRAY_SIZE(ip_vs_ops));
> > cleanup_dev:
> > unregister_pernet_device(&ipvs_core_dev_ops);
> > cleanup_sub:
> > @@ -2014,6 +2022,7 @@ exit:
> >
> > static void __exit ip_vs_cleanup(void)
> > {
> > + ip_vs_unregister_nl_ioctl();
> > nf_unregister_hooks(ip_vs_ops, ARRAY_SIZE(ip_vs_ops));
> > unregister_pernet_device(&ipvs_core_dev_ops);
> > unregister_pernet_subsys(&ipvs_core_ops); /* free ip_vs struct */
> > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> > index 7131417..efaf484 100644
> > --- a/net/netfilter/ipvs/ip_vs_ctl.c
> > +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> > @@ -3750,21 +3750,10 @@ void __net_exit ip_vs_control_net_cleanup(struct net *net)
> > free_percpu(ipvs->tot_stats.cpustats);
> > }
> >
> > -int __init ip_vs_control_init(void)
> > +int ip_vs_register_nl_ioctl(void)
> > {
> > - int idx;
> > int ret;
> >
> > - EnterFunction(2);
> > -
> > - /* Initialize svc_table, ip_vs_svc_fwm_table, rs_table */
> > - for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
> > - INIT_LIST_HEAD(&ip_vs_svc_table[idx]);
> > - INIT_LIST_HEAD(&ip_vs_svc_fwm_table[idx]);
> > - }
> > -
> > - smp_wmb(); /* Do we really need it now ? */
> > -
> > ret = nf_register_sockopt(&ip_vs_sockopts);
> > if (ret) {
> > pr_err("cannot register sockopt.\n");
> > @@ -3776,28 +3765,47 @@ int __init ip_vs_control_init(void)
> > pr_err("cannot register Generic Netlink interface.\n");
> > goto err_genl;
> > }
> > -
> > - ret = register_netdevice_notifier(&ip_vs_dst_notifier);
> > - if (ret < 0)
> > - goto err_notf;
> > -
> > - LeaveFunction(2);
> > return 0;
> >
> > -err_notf:
> > - ip_vs_genl_unregister();
> > err_genl:
> > nf_unregister_sockopt(&ip_vs_sockopts);
> > err_sock:
> > return ret;
> > }
> >
> > +void ip_vs_unregister_nl_ioctl(void)
>
> There is a extra trailing whitespace behind
> ip_vs_unregister_nl_ioctl(void)
>
>
> > +{
> > + ip_vs_genl_unregister();
> > + nf_unregister_sockopt(&ip_vs_sockopts);
> > +}
> > +
> > +int __init ip_vs_control_init(void)
> > +{
> > + int idx;
> > + int ret;
> > +
> > + EnterFunction(2);
> > +
> > + /* Initialize svc_table, ip_vs_svc_fwm_table, rs_table */
> > + for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
>
> The for loop is funny spaced...
>
>
> > + INIT_LIST_HEAD(&ip_vs_svc_table[idx]);
> > + INIT_LIST_HEAD(&ip_vs_svc_fwm_table[idx]);
> > + }
> > +
> > + smp_wmb(); /* Do we really need it now ? */
> > +
> > + ret = register_netdevice_notifier(&ip_vs_dst_notifier);
> > + if (ret < 0)
> > + return ret;
> > +
> > + LeaveFunction(2);
> > + return 0;
> > +}
> > +
> >
> > void ip_vs_control_cleanup(void)
> > {
> > EnterFunction(2);
> > unregister_netdevice_notifier(&ip_vs_dst_notifier);
> > - ip_vs_genl_unregister();
> > - nf_unregister_sockopt(&ip_vs_sockopts);
> > LeaveFunction(2);
> > }
>
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>
^ 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