From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id EF70E199B0 for ; Wed, 17 Jan 2018 17:18:10 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jan 2018 08:18:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,372,1511856000"; d="scan'208";a="10988931" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by fmsmga007.fm.intel.com with ESMTP; 17 Jan 2018 08:18:08 -0800 To: Stephen Hemminger Cc: Andrew Rybchenko , dev@dpdk.org, Yuanhan Liu References: <20180116183755.24542-1-stephen@networkplumber.org> <42f4f5a2-008d-c3e8-4c00-ed9ef59065c8@solarflare.com> <27f327e5-5632-69cc-feaa-10cf9384a701@intel.com> <20180117080555.5afd8c78@xeon-e3> From: Ferruh Yigit Message-ID: <24375588-ec05-9e54-264d-69dcb48f7f16@intel.com> Date: Wed, 17 Jan 2018 16:18:07 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180117080555.5afd8c78@xeon-e3> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5 00/15] common ethdev linkstatus functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jan 2018 16:18:11 -0000 On 1/17/2018 4:05 PM, Stephen Hemminger wrote: > On Wed, 17 Jan 2018 14:32:17 +0000 > Ferruh Yigit wrote: > >> On 1/17/2018 7:56 AM, Andrew Rybchenko wrote: >>> On 01/16/2018 09:37 PM, Stephen Hemminger wrote: >>>> While reviewing drivers, noticed a lot of unnecessary >>>> duplication of code in drivers for handling the eth_dev link status >>>> information. While consolidating this, it also became obvious that >>>> some drivers behave differently for no good reason. >>>> >>>> It also was a good chance to introduce atomic exchange primitives >>>> in EAL because there are other places using cmpset where not >>>> necessary (such as bonding). >>>> >>>> Mostly only compile tested only, don't have all of the hardware >>>> available (except ixgbe and virtio) to test. >>>> >>>> Note: the eth_dev_link_update function return value is inconsistent >>>> across drivers. Should be changed to be void. >>> >>> I would say "link_update" callback return value is inconsistent across >>> drivers. I'm not sure which direction is right here: make it consistent >>> or make it void. Also any changes in link information could be >>> important. As I understand it should not happen without up/down, >>> but bugs with loss of intermediate transitions are definitely possible. >>> So, notifying about any changes in link information is definitely safer. >>> May be not now. >> >> Again, why not return previous link status, it is simple enough to prevent >> inconsistent usage. >> >> rte_eth_link_get() already discards the return value, so won't be a problem there. >> >> For the cases PMD would like know about link changes, they will need to >> implement almost same link_update function with a return value, so why not use >> existing link_update function? >> >> Like been in virtio, link_update() used in interrupt handler, and calls a >> callback process if status changes. When link_update() return status changed to >> void, I guess they will need to implement another version of the link_update >> with return and use it. > > The interrupt and non-interrupt model are different. Yes. But for virtio specific usage: virtio_interrupt_handler() virtio_dev_link_update() == 0 _rte_eth_dev_callback_process() meantime same exact virtio_dev_link_update() used as: .link_update = virtio_dev_link_update, so updating virtio_dev_link_update() to not return status change, will update logic in virtio_interrupt_handler(), no? > Also the driver internally may want to do something different, this is about > the return value for dev_ops->link_update. Agreed, driver may do something different. And the function needs to be implemented will be very close to dev_ops->link_update. I thought making dev_ops->link_update more generic can prevent duplication there. And aligns with virtio usage.. > The code in rte_eth_dev never > used the return value. The bonding driver was expecting it to work but it > doesn't. Agreed. > Anyway drivers shouldn't in general be directly calling other > devices eth_dev_ops I guess now there are a few overlay PMDs does this.