From owner-freebsd-net@freebsd.org Fri Dec 18 22:11:25 2015 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 66E89A4BBBA for ; Fri, 18 Dec 2015 22:11:25 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: from phabric-backend.rbsd.freebsd.org (unknown [IPv6:2607:fc50:2000:101::1bb:73]) by mx1.freebsd.org (Postfix) with ESMTP id 53B0D14B4 for ; Fri, 18 Dec 2015 22:11:25 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: by phabric-backend.rbsd.freebsd.org (Postfix, from userid 1346) id 4EE721180D; Fri, 18 Dec 2015 22:11:25 +0000 (UTC) Date: Fri, 18 Dec 2015 22:11:25 +0000 To: freebsd-net@freebsd.org From: "hrs (Hiroki Sato)" Reply-to: D1986+325+381818416dc12ca2@reviews.freebsd.org Subject: [Differential] [Commented On] D1986: Teach lagg(4) to change MTU Message-ID: X-Priority: 3 X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: Thread-Topic: D1986: Teach lagg(4) to change MTU X-Herald-Rules: none, <28> X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: Precedence: bulk In-Reply-To: References: Thread-Index: ODZhMzNlYThiYzMxOTgzYmRhMDE5M2Q2Yzk4IFZ0hI0= MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.20 List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Dec 2015 22:11:25 -0000 hrs added inline comments. INLINE COMMENTS sys/net/if_lagg.c:753 Please separate a llq loop from a handler for per-port configuration. A llq traversal should be required only once in lagg_port_ops() if the handlers process a single lagg_llq entry. sys/net/if_lagg.c:837 Is this (llq == NULL), not (llq != NULL)? sys/net/if_lagg.c:840 Why is cleanup required here? This removes all of tasks not limited to MTU change. sys/net/if_lagg.c:861 free(NULL) does nothing. Checking if NULL or not is useless. sys/net/if_lagg.c:872 This traversal and freeing an entry after processing it should be done in lagg_port_ops(). sys/net/if_lagg.h:220 Please add "llq_" prefix to the members. sys/net/if_lagg.h:221 Is there any reason to have ifr as a pointer? malloc is generally expensive in kernel, and overhead of struct ifreq is acceptable for me even if every llq has one. I feel this complicates the error handling at least. REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D1986 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: rpokala, rstone, rpokala-panasas.com Cc: smh, imp, melifaro, hrs, sbruno, lakshmi.n_msystechnologies.com, emaste, ae, freebsd-net-list