From: Ilya Maximets <i.maximets@samsung.com>
To: "Wiles, Keith" <keith.wiles@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Wu, Jingjing" <jingjing.wu@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
"Xing, Beilei" <beilei.xing@intel.com>,
"Zhang, Qi Z" <qi.z.zhang@intel.com>,
"Wang, Xiao W" <xiao.w.wang@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [RFC 2/2] drivers/net: use sleep delay by default for intel NICs
Date: Mon, 3 Sep 2018 18:50:12 +0300 [thread overview]
Message-ID: <20180903154841eucas1p1016af55e5cd460a7e1029e20ad5c5e4a~Q7oN_vC0l2380923809eucas1p1N@eucas1p1.samsung.com> (raw)
In-Reply-To: <832A4FA5-2615-4CD1-8B48-0176557BE6E6@intel.com>
On 03.09.2018 18:39, Wiles, Keith wrote:
>
>
>> On Sep 3, 2018, at 4:33 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> On 03.09.2018 18:14, Wiles, Keith wrote:
>>>
>>>
>>>> On Aug 31, 2018, at 1:45 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>
>>>> NICs uses different delays up to a second during their
>>>> configuration. It makes no sense to busy-wait so long wasting
>>>> CPU cycles and preventing any other threads to execute on the
>>>> same CPU core. These busy polling are the rudiments that came
>>>> from the kernel drivers where you can not sleep in interrupt
>>>> context, but as we're in userspace, we're able and should
>>>> sleep to allow other threads to run.
>>>> Delays never called on rx/tx path, so this should not affect
>>>> performance.
>>>
>>> I have a question, the only thread being effected by the busy wait is the thread assigned to the core or the master lcore in the case of rte_eal_init(). It should not effect other threads running on other cores, right? If you do have other threads running in the same code then I see the need, please help me understand the use and how it is effecting DPDK.
>>
>> Originally, this patch comes from this issue:
>> http://mails.dpdk.org/archives/dev/2018-August/110640.html
>>
>> non-EAL threads is one of the answers.
>> For example, main thread in OVS periodically checks the link statuses
>> and hangs there busy waiting on different NIC configuration operations.
>
> We have link state get no wait why is that not working instead of wait polling the line state?
Yes, and OVS uses it, but ixgbe driver contains the workaround for a
fiber links configuration issue that calls ixgbe_setup_link, that
busy waits trying to configure multispeed fiber. You may found
details in the patch I mentioned. I moved this code to the separate
alarm thread, but it will eat CPU cycles anyway preventing others
from working.
>
>> Also, main OVS tread is responsible for port configuration and
>> re-configuration. There are few other in-dpdk threads like interrupts
>> handling thread (alarm handling thread). vhost_thread is working on
>> the same lcores and wants some time to work too.
>>
>> In case of hyperthreading busy-waiting will significantly affect
>> threads on the siblings.
>>
>> Best regards, Ilya Maximets.
>>
>>>>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> ---
>>>> drivers/net/avf/Makefile | 1 +
>>>> drivers/net/avf/base/avf_osdep.h | 4 ++--
>>>> drivers/net/e1000/Makefile | 1 +
>>>> drivers/net/e1000/base/e1000_osdep.h | 2 +-
>>>> drivers/net/i40e/base/i40e_osdep.h | 6 +++---
>>>> drivers/net/ifc/base/ifcvf_osdep.h | 2 +-
>>>> drivers/net/ixgbe/base/ixgbe_osdep.h | 2 +-
>>>> 7 files changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/avf/Makefile b/drivers/net/avf/Makefile
>>>> index 3f815bbc4..8ee707529 100644
>>>> --- a/drivers/net/avf/Makefile
>>>> +++ b/drivers/net/avf/Makefile
>>>> @@ -9,6 +9,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>>> LIB = librte_pmd_avf.a
>>>>
>>>> CFLAGS += -O3
>>>> +CFLAGS += -DALLOW_EXPERIMENTAL_API
>>>> LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
>>>> LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs -lrte_hash
>>>> LDLIBS += -lrte_bus_pci
>>>> diff --git a/drivers/net/avf/base/avf_osdep.h b/drivers/net/avf/base/avf_osdep.h
>>>> index 9ef45968e..442a5acd0 100644
>>>> --- a/drivers/net/avf/base/avf_osdep.h
>>>> +++ b/drivers/net/avf/base/avf_osdep.h
>>>> @@ -93,8 +93,8 @@ typedef uint64_t u64;
>>>> #define avf_memset(a, b, c, d) memset((a), (b), (c))
>>>> #define avf_memcpy(a, b, c, d) rte_memcpy((a), (b), (c))
>>>>
>>>> -#define avf_usec_delay(x) rte_delay_us(x)
>>>> -#define avf_msec_delay(x) rte_delay_us(1000*(x))
>>>> +#define avf_usec_delay(x) rte_delay_us_sleep(x)
>>>> +#define avf_msec_delay(x) avf_usec_delay(1000 * (x))
>>>>
>>>> #define AVF_PCI_REG(reg) rte_read32(reg)
>>>> #define AVF_PCI_REG_ADDR(a, reg) \
>>>> diff --git a/drivers/net/e1000/Makefile b/drivers/net/e1000/Makefile
>>>> index 9c87e883b..0ed627656 100644
>>>> --- a/drivers/net/e1000/Makefile
>>>> +++ b/drivers/net/e1000/Makefile
>>>> @@ -10,6 +10,7 @@ LIB = librte_pmd_e1000.a
>>>>
>>>> CFLAGS += -O3
>>>> CFLAGS += $(WERROR_FLAGS)
>>>> +CFLAGS += -DALLOW_EXPERIMENTAL_API
>>>> LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
>>>> LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs
>>>> LDLIBS += -lrte_bus_pci
>>>> diff --git a/drivers/net/e1000/base/e1000_osdep.h b/drivers/net/e1000/base/e1000_osdep.h
>>>> index b8868049f..5958ea157 100644
>>>> --- a/drivers/net/e1000/base/e1000_osdep.h
>>>> +++ b/drivers/net/e1000/base/e1000_osdep.h
>>>> @@ -48,7 +48,7 @@
>>>>
>>>> #include "../e1000_logs.h"
>>>>
>>>> -#define DELAY(x) rte_delay_us(x)
>>>> +#define DELAY(x) rte_delay_us_sleep(x)
>>>> #define usec_delay(x) DELAY(x)
>>>> #define usec_delay_irq(x) DELAY(x)
>>>> #define msec_delay(x) DELAY(1000*(x))
>>>> diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
>>>> index 8e5c593c9..a6072e153 100644
>>>> --- a/drivers/net/i40e/base/i40e_osdep.h
>>>> +++ b/drivers/net/i40e/base/i40e_osdep.h
>>>> @@ -233,9 +233,9 @@ struct i40e_spinlock {
>>>> #define i40e_memcpy(a, b, c, d) rte_memcpy((a), (b), (c))
>>>>
>>>> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
>>>> -#define DELAY(x) rte_delay_us(x)
>>>> -#define i40e_usec_delay(x) rte_delay_us(x)
>>>> -#define i40e_msec_delay(x) rte_delay_us(1000*(x))
>>>> +#define DELAY(x) rte_delay_us_sleep(x)
>>>> +#define i40e_usec_delay(x) DELAY(x)
>>>> +#define i40e_msec_delay(x) DELAY(1000 * (x))
>>>> #define udelay(x) DELAY(x)
>>>> #define msleep(x) DELAY(1000*(x))
>>>> #define usleep_range(min, max) msleep(DIV_ROUND_UP(min, 1000))
>>>> diff --git a/drivers/net/ifc/base/ifcvf_osdep.h b/drivers/net/ifc/base/ifcvf_osdep.h
>>>> index cf151ef52..6aef25ea4 100644
>>>> --- a/drivers/net/ifc/base/ifcvf_osdep.h
>>>> +++ b/drivers/net/ifc/base/ifcvf_osdep.h
>>>> @@ -17,7 +17,7 @@
>>>> #define DEBUGOUT(S, args...) RTE_LOG(DEBUG, PMD, S, ##args)
>>>> #define STATIC static
>>>>
>>>> -#define msec_delay rte_delay_ms
>>>> +#define msec_delay(x) rte_delay_us_sleep(1000 * (x))
>>>>
>>>> #define IFCVF_READ_REG8(reg) rte_read8(reg)
>>>> #define IFCVF_WRITE_REG8(val, reg) rte_write8((val), (reg))
>>>> diff --git a/drivers/net/ixgbe/base/ixgbe_osdep.h b/drivers/net/ixgbe/base/ixgbe_osdep.h
>>>> index bb5dfd2af..94ede9bc2 100644
>>>> --- a/drivers/net/ixgbe/base/ixgbe_osdep.h
>>>> +++ b/drivers/net/ixgbe/base/ixgbe_osdep.h
>>>> @@ -51,7 +51,7 @@
>>>>
>>>> #define ASSERT(x) if(!(x)) rte_panic("IXGBE: x")
>>>>
>>>> -#define DELAY(x) rte_delay_us(x)
>>>> +#define DELAY(x) rte_delay_us_sleep(x)
>>>> #define usec_delay(x) DELAY(x)
>>>> #define msec_delay(x) DELAY(1000*(x))
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>
>>> Regards,
>>> Keith
>
> Regards,
> Keith
>
>
>
next prev parent reply other threads:[~2018-09-03 15:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20180831124517.27619-1-i.maximets@samsung.com>
[not found] ` <CGME20180831124402eucas1p120ba1cdc8c3f6bc6c5bd09b5d3ede771@eucas1p1.samsung.com>
2018-08-31 12:45 ` [dpdk-dev] [RFC 1/2] eal: add nanosleep based delay function Ilya Maximets
[not found] ` <CGME20180831124404eucas1p20daff43600dfe450c9106616f886eab4@eucas1p2.samsung.com>
2018-08-31 12:45 ` [dpdk-dev] [RFC 2/2] drivers/net: use sleep delay by default for intel NICs Ilya Maximets
2018-09-03 15:14 ` Wiles, Keith
2018-09-03 15:33 ` Ilya Maximets
2018-09-03 15:39 ` Wiles, Keith
2018-09-03 15:50 ` Ilya Maximets [this message]
2018-09-03 16:04 ` Wiles, Keith
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='20180903154841eucas1p1016af55e5cd460a7e1029e20ad5c5e4a~Q7oN_vC0l2380923809eucas1p1N@eucas1p1.samsung.com' \
--to=i.maximets@samsung.com \
--cc=beilei.xing@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=keith.wiles@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=qi.z.zhang@intel.com \
--cc=wenzhuo.lu@intel.com \
--cc=xiao.w.wang@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).