DPDK patches and discussions
 help / color / mirror / Atom feed
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
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?

  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:

* 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 \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git