From: Neil Horman <nhorman@tuxdriver.com>
To: "Wodkowski, PawelX" <pawelx.wodkowski@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] bond: add mode 4 support
Date: Mon, 22 Sep 2014 06:24:55 -0400 [thread overview]
Message-ID: <20140922102455.GA25406@hmsreliant.think-freely.org> (raw)
In-Reply-To: <F6F2A6264E145F47A18AB6DF8E87425D12B2D6F9@IRSMSX102.ger.corp.intel.com>
On Mon, Sep 22, 2014 at 06:26:21AM +0000, Wodkowski, PawelX wrote:
> > I think that will work, but I believe you're making it more complicated (and
> > less reusable) than it needs to be. What I think you really need to do is
> > create a new rte api call, rte_eal_alarm_cancel_sync (something like the
> > equivalent of del_timer_sync in linux, that wraps up the
> > while(rte_eal_alarm_cancel(...) == 0) {rte_pause} in its own function (so other
> > call sites can use it, as I don't think this is an uncommon problem), Then just
> > create a bonding-internal state flag to signal the periodic callback that it
> > shouldn't re-arm the timer. That way all you have to do is set the flag, and
> > call rte_eal_alarm_cancel_sync, and you're done. And other applications will be
> > able to handle this common type of operation as well
> >
> > Neil
>
> I agree with you that alarms should be upgraded but this need to discussed and
> tested. Now there is not time for that.
>
Nak, thats a completely broken argument. I've just demonstrated to you a race
condition in the driver you are submitting. Granted it stems from a design
lmitation in the alarm subsystem, but its what we all have to work with, and we
can work around it and make the driver safe. To say there is "no time" to fix
it, implies to me that you care more about just slamming your code in than
making anything work properly. What exactly is your rush that makes you think
its more important for the code to be merged than fixing it to work correctly?
Neil
next prev parent reply other threads:[~2014-09-22 10:19 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-17 14:21 [dpdk-dev] [PATCH 0/2] " Pawel Wodkowski
2014-09-17 14:21 ` [dpdk-dev] [PATCH 1/2] bond: extract common code to separate functions Pawel Wodkowski
2014-09-17 14:51 ` Neil Horman
2014-09-17 14:21 ` [dpdk-dev] [PATCH 2/2] bond: add mode 4 support Pawel Wodkowski
2014-09-17 15:13 ` Neil Horman
2014-09-18 8:07 ` Wodkowski, PawelX
2014-09-18 16:02 ` Neil Horman
2014-09-19 12:47 ` Wodkowski, PawelX
2014-09-19 17:29 ` Neil Horman
2014-09-22 6:26 ` Wodkowski, PawelX
2014-09-22 10:24 ` Neil Horman [this message]
[not found] ` <60ABE07DBB3A454EB7FAD707B4BB158213896977@IRSMSX109.ger.corp.intel.com>
2014-09-22 13:15 ` Neil Horman
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=20140922102455.GA25406@hmsreliant.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=dev@dpdk.org \
--cc=michalx.k.jastrzebski@intel.com \
--cc=pawelx.wodkowski@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).