DPDK patches and discussions
 help / color / mirror / Atom feed
From: Matan Azrad <matan@mellanox.com>
To: Chas Williams <3chas3@gmail.com>, Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "Doherty, Declan" <declan.doherty@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"stable@dpdk.org" <stable@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] net/bonding: fix slave activation simultaneously
Date: Tue, 15 May 2018 08:24:26 +0000	[thread overview]
Message-ID: <VI1PR0501MB2608B1300011124A8D6EEC40D2930@VI1PR0501MB2608.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <CAG2-GkkOkB5EVAn1_PdwQc1et=qL2vQrj6BPeRXrdWwydkjTDQ@mail.gmail.com>

Hi

From: Chas Williams
> There's possibly an issue here:
> 
>         /* If the device isn't started don't handle interrupts */
>         if (!bonded_eth_dev->data->dev_started)
>                 return rc;
> 
>         /* verify that port_id is a valid slave of bonded port */
>         for (i = 0; i < internals->slave_count; i++) {
>                 if (internals->slaves[i].port_id == port_id) {
>                         valid_slave = 1;
>                         break;
>                 }
>         }
> 
>         if (!valid_slave)
>                 return rc;
> 
>         /* Synchronize lsc callback parallel calls either by real link event
>          * from the slaves PMDs or by the bonding PMD itself.
>          */
>         rte_spinlock_lock(&internals->lsc_lock);
> 
> Nothing keeps the master thread from modifying internals while this is running
> and your slave port may suddenly no longer be a slave port by the time you get
> to lsc_lock.
> I think there is probably an issue dev_started as well.  Again, nothing keeps the
> LSC thread from attempting to activate a slave that may have just been
> stopped/removed.

Yes, you right, the same issue for data-path functions, I think there is a big synchronization work that should be done for the bonding PMD.
This patch just synchronizes the callback parallel calls and solved an issue saw in mlx5 device, and do not pretend to solve all the sync issues.

> 
> On Mon, May 14, 2018 at 8:41 AM, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> On 5/14/2018 12:45 PM, Doherty, Declan wrote:
> > On 24/04/2018 12:29 PM, Matan Azrad wrote:
> >> The bonding PMD decides to activate\deactivate its slaves according
> >> to the slaves link statuses.
> >> Thus, it registers to the LSC events of the slaves ports and
> >> activates\deactivates them from its LSC callbacks called
> >> asynchronously by the host thread when the slave link status is changed.
> >>
> >> In addition, the bonding PMD uses the callback for slave activation
> >> when it tries to start it, this operation is probably called by the
> >> master thread.
> >>
> >> Consequently, a slave may be activated in the same time by two
> >> different threads and may cause a lot of optional errors, for
> >> example, slave mempool recreation with the same name causes an error.
> >>
> >> Synchronize the critical section in the LSC callback using a special
> >> new spinlock.
> >>
> >> Fixes: 414b202343ce ("bonding: fix initial link status of slave")
> >> Fixes: a45b288ef21a ("bond: support link status polling")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >> ---
> > ...
> >>
> >
> > Acked-by: Declan Doherty <declan.doherty@intel.com>
> >
> Applied to dpdk-next-net/master, thanks.
> 



      reply	other threads:[~2018-05-15  8:24 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-24 11:29 [dpdk-dev] " Matan Azrad
2018-05-14  2:07 ` Thomas Monjalon
2018-05-14 11:45 ` Doherty, Declan
2018-05-14 12:41   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2018-05-14 15:01     ` Chas Williams
2018-05-15  8:24       ` Matan Azrad [this message]

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=VI1PR0501MB2608B1300011124A8D6EEC40D2930@VI1PR0501MB2608.eurprd05.prod.outlook.com \
    --to=matan@mellanox.com \
    --cc=3chas3@gmail.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /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).