[dpdk-dev] [PATCH v3 00/34] A new net PMD - ice

Varghese, Vipin vipin.varghese at intel.com
Fri Dec 14 04:26:21 CET 2018


Hi Wenzhuo,

snipped
> >
> > Thanks for the update, couple of suggestion in my opinion shared below
> >
> > Snipped
> > > > [PATCH v2 02/20] net/ice: support device initialization
> > > > > +# Compile burst-oriented ICE PMD driver #
> > > > CONFIG_RTE_LIBRTE_ICE_PMD=y
> > > >
> > > > Based on ' https://patches.dpdk.org/patch/48488/' it is suggested
> > > > to configure. But here is it already set to 'y'. Is this correct?
> > > > If yes can you update ' https://patches.dpdk.org/patch/48488/'
> > > We've discussed the setting. I don’t know anything left. If there's,
> > > please let me know.
> > I think I poorly communicated, so let me try again. In document it is
> > stated to configure as 'y'. But in your default config is 'y'. So
> > either make the default as 'n' so user can configure or reword in
> > document as 'Ensure the config is set to y for before building'
> In the patch, I only see "``CONFIG_RTE_LIBRTE_ICE_PMD`` (default ``y``) ". No
> conflict with the configure file.
I think I am failing you to convey even after multiple attempts. Hence I humbly leave this to other from mailing list to get it sorted.

> 
> >
> > >
> > > >
> > > >  [PATCH v2 20/20] net/ice: support meson build
> > > > > > > > Should not meson build option be add start. That is in
> > > > > > > > patch
> > > > > > > > 1/20 so compile options does not fail?
> > > > > > > It will not fail. Enabling the compile earlier only means
> > > > > > > the code can be
> > > > > > compiled.
> > > > > > > But, to use this device we do need the whole patch set. From
> > > > > > > this point of view, compiling it at the end maybe better.
> > > > > > Thanks for update, so will 'meson-build' success if apply 3 patches?
> > > > > Sure, meson build will not be broken by any one of these patches.
> > > > > Only until this patch, what built by meson can support ice.
> > > > Thanks for confirmation that you have tried
> > > > './devtools/test-meson- builds.sh' and the intermediate build for ICE_DSI
> PMD does not fail.
> > > I said meson build is working. But don't know what's ICE_DSI PMD.
> >
> > Once again apologies if I poorly communicated ICE_PMD, my question is
> > 'if I take patch 1/20 in v2 can I build for librte_pmd_ice'? is not we
> > are expecting each additional layering for functionality like mtu set,
> > switch set should be compiling and not just the last build?
> O, you want to support meson build from the beginning. I can move it forward,
> although I recommend to use it after all the patches.
Thank you for consideration, I think this approach helps a lot since we are systematically adding code to ICE_PMD

> 
> >
> > >
> > > >
> > > >  [PATCH v2 16/20] net/ice: support basic RX/TX
> > > >
> > > >  [PATCH v2 14/20] net/ice: support statistics
> > > >  > > > +	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns-
> > >eth.tx_multicast
> > > +
> > > > > > > +			     ns->eth.tx_broadcast) * ETHER_CRC_LEN;
> > > > > >
> > > > > > In earlier patch for 'mtu set check' we added VSI SWITCH VLAN.
> > > > > > Should we add VSI VLAN here?
> > > > > Don't need. They're different functions. We add crc length here
> > > > > because of HW counts the packet length before crc is added.
> > > > So you are not fetching stats from HW registers from switch is
> > > > this
> > correct?
> > > > How will you get stats for actually transmitted in xstats? As I
> > > > understand xstats is for switch HW stats right?
> > > No. The stats is got from HW. We just correct it as HW doesn’t have
> > > the chance to add CRC length. But I don't understand why switch is
> > mentioned here.
> > Will ice pmd  for CVL expose all the switch stats as 'xstats'? If yes,
> > can you share in patch comments what are switch level stats?
> No. xstats means the extend stats which is not covered by the basic stats. It can be
> anything which is not in basic stats.
Thanks for confirming, hence is xstats covering for ICE Switch stats which is not covered in PMD stats? If yes, can you mention the list of stats covered under xstats that is fetched specifically from ICE switch?

> 
> >
> > >
> > > >
> > > >  [PATCH v2 04/20] net/ice: support getting	device	information
> > > >  > > Does this mean per queue offload capability is not supported?
> > > > If
> > > > > > yes, can you mention this in release notes under 'support or
> > limitation'
> > > > > No, it's not supported. We have a document, ice.ini, to list all
> > > > > the features supported. All the others are not supported.
> > > > > BTW, I don't think anything not supported is limitation.
> > > > If I understand correctly,  ICE_DSI_PMD is advertising it has not
> > > > offload for RX and TX. But you are stating in ice.ini you are
> > > > listing offload supports. So let me rephrase the question 'if you
> > > > support port level offload capability, it will reflect for all queues rx and tx.
> > > > But if you reflect queue level offload as 0 for rx and tx, then
> > > > APIs rte_eth_rx_queue_setup and rte_eth_tx_queue_setup if queue
> > > > offload enabled should fail. Is this correct understanding?'
> > > Sorry, I don’t know what's ICE_DSI_PMD.
> > ICE_PMD
> No exactly. We follow the rule of queue and port offload setting. If the feature is
> enabled in port level, we can ignore the queue setting. If the feature is not
> enabled in port level, we still can enable it per queue.
Thanks for confirming, so what will be result for non-configured ICE port for offload for port and queue using rte_eth_dev_get_info?

> 
> >
> >
> > >
> > > >
> > > > > > If device switch is not configured (default value from NVM)
> > > > > > should we highlight the switch can support speed 10, 100,
> > > > > > 1000,
> > > > > > 1000 and son
> > > > on?
> > > > > No, this's the capability getting from HW.
> > > > If HW is supported or configured for 10, 100, 25G then those
> > > > should be returned correctly this I agree. But when the device is
> > > > queried for capability it should highlight all supported speeds of switch. Am I
> right?
> > > No. Here shows the result not all the speeds supported. Like the
> > > speed after auto negotiation.
> > As per your current statement "If user uses API rte_eth_dev_info_get
> > to get speed_capa, current speed will be returned as auto negotiated
> > value and not ' ETH_LINK_SPEED_10M| ETH_LINK_SPEED_100M|
> > ETH_LINK_SPEED_1G| ETH_LINK_SPEED_10G| ETH_LINK_SPEED_25G'". I will
> > leave this to others to comment since in my humble opinion this is not
> > expected.
> OK. I'll change it to the bitmap.
Thanks, appreciate the understanding

> 
> >
> > >
> > > >
> > > > > > If speed is not true as stated above, can you please add this
> > > > > > to release notes and documentation.
> > > > > Here listed all the case we can get from HW.
> > > > Please add to ice_dsi documentation also.
> > > Sorry, no idea about ice_dsi.
> > My mistake ICE_PMD is what I am referring to.
> >
> > >
> > > >
> > > > snipped



More information about the dev mailing list