From owner-freebsd-net@freebsd.org Wed Dec 21 21:37:44 2016 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 8CA94C8BC69 for ; Wed, 21 Dec 2016 21:37:44 +0000 (UTC) (envelope-from v.maffione@gmail.com) Received: from mail-oi0-x244.google.com (mail-oi0-x244.google.com [IPv6:2607:f8b0:4003:c06::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CB851EA8; Wed, 21 Dec 2016 21:37:44 +0000 (UTC) (envelope-from v.maffione@gmail.com) Received: by mail-oi0-x244.google.com with SMTP id f201so30051994oib.0; Wed, 21 Dec 2016 13:37:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=9DqsNdVjOtMNfOonX+jOJuZYSzQskHmFcNtwbEpFTdU=; b=g2IYFAqxXf9vgRMcTW4LMBR88KwbVRz3uslxrfpBuE6mfWtteeGyaHWh0Pu2T77kSv 4IGPRoa4jIIfJCU92QE2DcMeWMwhu2SyRsJrBq8bn3f2+PBj5tJvj9dKUncDpjoq19gH lrQTWUkpSLzXSKjdrbG91CmuveYyWxyus2ueCTTR3Srs7mQDoeAOLaykataXsgplQ62O EpjAcHzsGjIK0T9UyNMgEzg0MxbBQOO9OcVRR2SPQLwJAZugs2voaSno2EILPwaTMxMD NpLJt/o25VCkDzaJJG1v40L4UO4thhavCQ6t3Ua/xC7lOxrtely4zEX05z/Lp2j/Wvk5 H2uQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=9DqsNdVjOtMNfOonX+jOJuZYSzQskHmFcNtwbEpFTdU=; b=i9wHKKxhppmjwnR2u3jYGrDKpccr9M9ZxikRizyhdtAQrzl6ZoKb1ehwlWIN8S1eHm 6R23Vr2g1wr99L1iKVC6D6Z7IxZMaLoyQM6kWMGPSj+AaDMKBk4hknOqN1UQ9z2o1I4S PhWcf/WUdbZSt2k3P2bQdNEZZ4lkR90VufxoQ1bZgz79NxidhC6D+Sh1bcjnBpnVWrth mx5zONz9OrQGPCV5ei/VwIZTivaH3jyKoYWfGhrFkhaWlSGUpiFUjxGDiIbSB9tSpvrr Uf/iC17VBVxfEiGomLNFjuCg+QabGtdfwtKN6/9ws8LPqr7B7F+6lZ3erg84kcOeTsUu S83g== X-Gm-Message-State: AIkVDXJGr8JGz3AajhO+gaMqFGH9TYsWfJUERJBIGVqlExMTfDsGVcSI4w7Z7QKa2CEfPCb36H9UpVzroKVtXw== X-Received: by 10.202.107.76 with SMTP id g73mr3647348oic.1.1482356263401; Wed, 21 Dec 2016 13:37:43 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.45.200 with HTTP; Wed, 21 Dec 2016 13:37:42 -0800 (PST) In-Reply-To: References: <20161217222812.GA4979@ox> From: Vincenzo Maffione Date: Wed, 21 Dec 2016 22:37:42 +0100 Message-ID: Subject: Re: cxgbe's native netmap support broken since r307394 To: Luigi Rizzo Cc: Adrian Chadd , Navdeep Parhar , "freebsd-net@freebsd.org" , Luigi Rizzo , Giuseppe Lettieri Content-Type: multipart/mixed; boundary=001a11417c3882e509054431f6ed X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Dec 2016 21:37:44 -0000 --001a11417c3882e509054431f6ed Content-Type: text/plain; charset=UTF-8 Hi Luigi, I attached a minimal change containing two fixes: - change IFNET_WLOCK into IFNET_RLOCK, to fix the cxgbe issue related to this thread - use the proper locking functions for the "worker_lock", unrelated but needed to avoid the O.S. to trap because of a mismatch between MTX_SPIN and MTX_DEF. Cheers, Vincenzo 2016-12-21 20:30 GMT+01:00 Luigi Rizzo : > On Wed, Dec 21, 2016 at 11:15 AM, Vincenzo Maffione > wrote: >> Hi, >> There is no commit related to that in the FreeBSD svn or git. >> >> The fix has been published to the github netmap repository here >> (branch master): https://github.com/luigirizzo/netmap >> >> What we should do is to import all the recent updates from the github >> into HEAD. I can prepare a patch for HEAD, if you wish. Just let me >> know. > > I just checked and the diff between FreeBSD head and netmap head > in github is almost 3k lines due to a lot of recent refactoring. > So, if there is an easy way to extract just the locking change that would > be preferable as an interim solution. > > cheers > luigi > >> >> Cheers, >> Vincenzo >> >> >> 2016-12-20 21:45 GMT+01:00 Adrian Chadd : >>> hi, >>> >>> What's the commit? We should get it into -HEAD asap. >>> >>> >>> -adrian >>> >>> >>> On 20 December 2016 at 01:25, Vincenzo Maffione wrote: >>>> Ok, applied to the netmap github repo. >>>> This fix will be published when Luigi does the next commit on FreeBSD. >>>> >>>> Cheers, >>>> Vincenzo >>>> >>>> 2016-12-19 20:05 GMT+01:00 Navdeep Parhar : >>>>> IFNET_RLOCK will work, thanks. >>>>> >>>>> Navdeep >>>>> >>>>> On Mon, Dec 19, 2016 at 3:21 AM, Vincenzo Maffione wrote: >>>>>> Hi Navdeep, >>>>>> >>>>>> Indeed, we have reviewed the code, and we think it is ok to >>>>>> implement nm_os_ifnet_lock() with IFNET_RLOCK(), instead of using >>>>>> IFNET_WLOCK(). >>>>>> Since IFNET_RLOCK() results into sx_slock(), this should fix the issue. >>>>>> >>>>>> On FreeBSD, this locking is needed to protect a flag read by nm_iszombie(). >>>>>> However, on Linux the same lock is also needed to protect the call to >>>>>> the nm_hw_register() callback, so we prefer to have an "unified" >>>>>> locking scheme, i.e. always calling nm_hw_register under the lock. >>>>>> >>>>>> Does this make sense to you? Would it be easy for you to make a quick >>>>>> test by replacing IFNET_WLOCK with IFNET_RLOCK? >>>>>> >>>>>> Thanks, >>>>>> Vincenzo >>>>>> >>>>>> 2016-12-17 23:28 GMT+01:00 Navdeep Parhar : >>>>>>> Luigi, Vincenzo, >>>>>>> >>>>>>> The last major update to netmap (r307394 and followups) broke cxgbe's >>>>>>> native netmap support. The problem is that netmap_hw_reg now holds an >>>>>>> rw_lock around the driver's netmap_on/off routines. It has always been >>>>>>> safe for the driver to sleep during these operations but now it panics >>>>>>> instead. >>>>>>> >>>>>>> Why is IFNET_WLOCK needed here? It seems like a regression to disallow >>>>>>> sleep on the control path. >>>>>>> >>>>>>> Regards, >>>>>>> Navdeep >>>>>>> >>>>>>> begin_synchronized_op with the following non-sleepable locks held: >>>>>>> exclusive rw ifnet_rw (ifnet_rw) r = 0 (0xffffffff8271d680) locked @ >>>>>>> /root/ws/head/sys/dev/netmap/netmap_freebsd.c:95 >>>>>>> stack backtrace: >>>>>>> #0 0xffffffff810837a5 at witness_debugger+0xe5 >>>>>>> #1 0xffffffff81084d88 at witness_warn+0x3b8 >>>>>>> #2 0xffffffff83ef2bcc at begin_synchronized_op+0x6c >>>>>>> #3 0xffffffff83f14beb at cxgbe_netmap_reg+0x5b >>>>>>> #4 0xffffffff809846f1 at netmap_hw_reg+0x81 >>>>>>> #5 0xffffffff809806de at netmap_do_regif+0x19e >>>>>>> #6 0xffffffff8098121d at netmap_ioctl+0x7ad >>>>>>> #7 0xffffffff8098682f at freebsd_netmap_ioctl+0x5f >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Vincenzo Maffione >>>>>> _______________________________________________ >>>>>> freebsd-net@freebsd.org mailing list >>>>>> https://lists.freebsd.org/mailman/listinfo/freebsd-net >>>>>> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" >>>> >>>> >>>> >>>> -- >>>> Vincenzo Maffione >>>> _______________________________________________ >>>> freebsd-net@freebsd.org mailing list >>>> https://lists.freebsd.org/mailman/listinfo/freebsd-net >>>> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" >> >> >> >> -- >> Vincenzo Maffione > > > > -- > -----------------------------------------+------------------------------- > Prof. Luigi RIZZO, rizzo@iet.unipi.it . Dip. di Ing. dell'Informazione > http://www.iet.unipi.it/~luigi/ . Universita` di Pisa > TEL +39-050-2217533 . via Diotisalvi 2 > Mobile +39-338-6809875 . 56122 PISA (Italy) > -----------------------------------------+------------------------------- -- Vincenzo Maffione --001a11417c3882e509054431f6ed Content-Type: text/x-patch; charset=US-ASCII; name="netmap-fix.patch" Content-Disposition: attachment; filename="netmap-fix.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_iwzglhe60 ZGlmZiAtLWdpdCBhL3N5cy9kZXYvbmV0bWFwL25ldG1hcF9mcmVlYnNkLmMgYi9zeXMvZGV2L25l dG1hcC9uZXRtYXBfZnJlZWJzZC5jCmluZGV4IGZkNDU5MmI3YjVhLi41MTViZTY2NDhlYSAxMDA2 NDQKLS0tIGEvc3lzL2Rldi9uZXRtYXAvbmV0bWFwX2ZyZWVic2QuYworKysgYi9zeXMvZGV2L25l dG1hcC9uZXRtYXBfZnJlZWJzZC5jCkBAIC05Miw3ICs5Miw3IEBAIG5tX29zX3NlbGluZm9fdW5p bml0KE5NX1NFTElORk9fVCAqc2kpCiB2b2lkCiBubV9vc19pZm5ldF9sb2NrKHZvaWQpCiB7Ci0J SUZORVRfV0xPQ0soKTsKKwlJRk5FVF9STE9DSygpOwogfQogCiB2b2lkCkBAIC0xMDIxLDEyICsx MDIxLDEyIEBAIG5tX29zX2t0aHJlYWRfd2FrZXVwX3dvcmtlcihzdHJ1Y3Qgbm1fa3RocmVhZCAq bm1rKQogCSAqIGJ1dCBzaW1wbHkgdGhhdCBpdCBoYXMgY2hhbmdlZCBzaW5jZSB0aGUgbGFzdAog CSAqIHRpbWUgdGhlIGt0aHJlYWQgc2F3IGl0LgogCSAqLwotCW10eF9sb2NrKCZubWstPndvcmtl cl9sb2NrKTsKKwltdHhfbG9ja19zcGluKCZubWstPndvcmtlcl9sb2NrKTsKIAlubWstPnNjaGVk dWxlZCsrOwogCWlmIChubWstPndvcmtlcl9jdHguY2ZnLndjaGFuKSB7CiAJCXdha2V1cCgodm9p ZCAqKSh1aW50cHRyX3Qpbm1rLT53b3JrZXJfY3R4LmNmZy53Y2hhbik7CiAJfQotCW10eF91bmxv Y2soJm5tay0+d29ya2VyX2xvY2spOworCW10eF91bmxvY2tfc3Bpbigmbm1rLT53b3JrZXJfbG9j ayk7CiB9CiAKIHZvaWQgaW5saW5lCkBAIC0xMDgwLDEwICsxMDgwLDEwIEBAIG5tX2t0aHJlYWRf d29ya2VyKHZvaWQgKmRhdGEpCiAJCQljdHgtPndvcmtlcl9mbihjdHgtPndvcmtlcl9wcml2YXRl KTsgLyogd29ya2VyIGJvZHkgKi8KIAkJfSBlbHNlIHsKIAkJCS8qIGNoZWNrcyBpZiB0aGVyZSBp cyBhIHBlbmRpbmcgbm90aWZpY2F0aW9uICovCi0JCQltdHhfbG9jaygmbm1rLT53b3JrZXJfbG9j ayk7CisJCQltdHhfbG9ja19zcGluKCZubWstPndvcmtlcl9sb2NrKTsKIAkJCWlmIChsaWtlbHko bm1rLT5zY2hlZHVsZWQgIT0gb2xkX3NjaGVkdWxlZCkpIHsKIAkJCQlvbGRfc2NoZWR1bGVkID0g bm1rLT5zY2hlZHVsZWQ7Ci0JCQkJbXR4X3VubG9jaygmbm1rLT53b3JrZXJfbG9jayk7CisJCQkJ bXR4X3VubG9ja19zcGluKCZubWstPndvcmtlcl9sb2NrKTsKIAogCQkJCWN0eC0+d29ya2VyX2Zu KGN0eC0+d29ya2VyX3ByaXZhdGUpOyAvKiB3b3JrZXIgYm9keSAqLwogCkBAIC0xMDk0LDcgKzEw OTQsNyBAQCBubV9rdGhyZWFkX3dvcmtlcih2b2lkICpkYXRhKQogCQkJCSAgICAmbm1rLT53b3Jr ZXJfbG9jaywgIm5ta19ldiIsIGh6KTsKIAkJCQlubWstPnNjaGVkdWxlZCsrOwogCQkJfQotCQkJ bXR4X3VubG9jaygmbm1rLT53b3JrZXJfbG9jayk7CisJCQltdHhfdW5sb2NrX3NwaW4oJm5tay0+ d29ya2VyX2xvY2spOwogCQl9CiAJfQogCg== --001a11417c3882e509054431f6ed--