From: Jesse Brandeburg <jesse.brandeburg@intel.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Stefan Assmann <sassmann@kpanic.de>,
intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
anthony.l.nguyen@intel.com, lihong.yang@intel.com,
slawomirx.laba@intel.com, nicholas.d.nunley@intel.com
Subject: Re: [PATCH] iavf: fix locking of critical sections
Date: Tue, 16 Mar 2021 15:02:10 -0700 [thread overview]
Message-ID: <20210316150210.00007249@intel.com> (raw)
In-Reply-To: <20210316132905.5d0f90dd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Jakub Kicinski wrote:
> > > I personally think that the overuse of flags in Intel drivers brings
> > > nothing but trouble. At which point does it make sense to just add a
> > > lock / semaphore here rather than open code all this with no clear
> > > semantics? No code seems to just test the __IAVF_IN_CRITICAL_TASK flag,
> > > all the uses look like poor man's locking at a quick grep. What am I
> > > missing?
> >
> > I agree with you that the locking could be done with other locking
> > mechanisms just as good. I didn't invent the current method so I'll let
> > Intel comment on that part, but I'd like to point out that what I'm
> > making use of is fixing what is currently in the driver.
>
> Right, I should have made it clear that I don't blame you for the
> current state of things. Would you mind sending a patch on top of
> this one to do a conversion to a semaphore?
>
> Intel folks any opinions?
I know Slawomir has been working closely with Stefan on figuring out
the right ways to fix this code. Hopefully he can speak for himself,
but I know he's on Europe time.
As for conversion to mutexes I'm a big fan, and as long as we don't
have too many collisions with the RTNL lock I think it's a reasonable
improvement to do, and if Stefan doesn't want to work on it, we can
look into whether Slawomir or his team can.
WARNING: multiple messages have this Message-ID (diff)
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH] iavf: fix locking of critical sections
Date: Tue, 16 Mar 2021 15:02:10 -0700 [thread overview]
Message-ID: <20210316150210.00007249@intel.com> (raw)
In-Reply-To: <20210316132905.5d0f90dd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Jakub Kicinski wrote:
> > > I personally think that the overuse of flags in Intel drivers brings
> > > nothing but trouble. At which point does it make sense to just add a
> > > lock / semaphore here rather than open code all this with no clear
> > > semantics? No code seems to just test the __IAVF_IN_CRITICAL_TASK flag,
> > > all the uses look like poor man's locking at a quick grep. What am I
> > > missing?
> >
> > I agree with you that the locking could be done with other locking
> > mechanisms just as good. I didn't invent the current method so I'll let
> > Intel comment on that part, but I'd like to point out that what I'm
> > making use of is fixing what is currently in the driver.
>
> Right, I should have made it clear that I don't blame you for the
> current state of things. Would you mind sending a patch on top of
> this one to do a conversion to a semaphore?
>
> Intel folks any opinions?
I know Slawomir has been working closely with Stefan on figuring out
the right ways to fix this code. Hopefully he can speak for himself,
but I know he's on Europe time.
As for conversion to mutexes I'm a big fan, and as long as we don't
have too many collisions with the RTNL lock I think it's a reasonable
improvement to do, and if Stefan doesn't want to work on it, we can
look into whether Slawomir or his team can.
next prev parent reply other threads:[~2021-03-16 22:03 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 10:01 [PATCH] iavf: fix locking of critical sections Stefan Assmann
2021-03-16 10:01 ` [Intel-wired-lan] " Stefan Assmann
2021-03-16 17:14 ` Jakub Kicinski
2021-03-16 17:14 ` [Intel-wired-lan] " Jakub Kicinski
2021-03-16 17:27 ` Stefan Assmann
2021-03-16 17:27 ` [Intel-wired-lan] " Stefan Assmann
2021-03-16 20:29 ` Jakub Kicinski
2021-03-16 20:29 ` [Intel-wired-lan] " Jakub Kicinski
2021-03-16 22:02 ` Jesse Brandeburg [this message]
2021-03-16 22:02 ` Jesse Brandeburg
2021-03-17 7:49 ` Stefan Assmann
2021-03-17 7:49 ` [Intel-wired-lan] " Stefan Assmann
2021-03-17 17:27 ` Laba, SlawomirX
2021-03-17 17:27 ` [Intel-wired-lan] " Laba, SlawomirX
2021-03-17 18:14 ` Stefan Assmann
2021-03-17 18:14 ` [Intel-wired-lan] " Stefan Assmann
2021-07-06 8:07 ` Jankowski, Konrad0
2021-07-06 8:07 ` Jankowski, Konrad0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210316150210.00007249@intel.com \
--to=jesse.brandeburg@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=kuba@kernel.org \
--cc=lihong.yang@intel.com \
--cc=netdev@vger.kernel.org \
--cc=nicholas.d.nunley@intel.com \
--cc=sassmann@kpanic.de \
--cc=slawomirx.laba@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.