Re: [dpdk-dev] [PATCH BUG 335 3/3] net/virtio: fix compilation error with 0 headroom

[Hemant Agrawal <hemant.agrawal@nxp.com>]

HI,
> On 7/26/19 2:25 PM, Olivier Matz wrote:
> > Hi,
> >
> > On Thu, Jul 25, 2019 at 04:36:45PM +0530, Hemant Agrawal wrote:
> >> When using RTE_PKTMBUF_HEADROOM as 0, virito ethdev driver throws
> >> compilation error
> >> virtio_ethdev.c:1851:2: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
> >> RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM
> >> 	< sizeof(struct virtio_net_hdr_mrg_rxbuf));
> >>
> >> This patch change it into run-time check.
> >> Reported as:
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug
> >>
> s.dpdk.org%2Fshow_bug.cgi%3Fid%3D335&amp;data=02%7C01%7Chemant.
> agrawa
> >>
> l%40nxp.com%7C303b7755f6ff4c44721a08d714f2cbb5%7C686ea1d3bc2b4c6
> fa92c
> >>
> d99c5c301635%7C0%7C0%7C637000905339982654&amp;sdata=oQ4b4TZ36x
> WiHOtKE
> >> %2BwCZfy2ND%2FgP%2FESfGL%2FEdoJbYI%3D&amp;reserved=0
> >>
> >> Fixes: 198ab33677c9 ("net/virtio: move device initialization in a
> >> function")
> >
> > I think the proper commit is:
> > Fixes: dec08c28c0b3 ("virtio: check packet headroom at compile time")
> 
> Indeed.
> 
> > It looks this patch more or less reverts this old commit.
> > +CC Stephen
> 
> I wonder whether we could have a warning at build time so that the one who
> builds DPDK is aware some driver may not be usable, in addition to the
> below patch that fails virtio-net init.

[Hemant] I will also prefer compile time check instead of run-time check for any non-default configs.
If someone is modifying the config, he can very well disable the drivers, which don't like those settings. 

However, earlier discussion w.r.t this bug moved in other direction to make DPDK compliable for all cases and allow regress testing.

> 
> Any thoughts?
> 
> Thanks,
> Maxime
> 
> >> Cc: stable@dpdk.org
> >> Cc: Olivier Matz <olivier.matz@6wind.com>
> >>
> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >> ---
> >>   drivers/net/virtio/virtio_ethdev.c | 9 ++++++++-
> >>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio/virtio_ethdev.c
> >> b/drivers/net/virtio/virtio_ethdev.c
> >> index 62c827443..3e2e8bd2a 100644
> >> --- a/drivers/net/virtio/virtio_ethdev.c
> >> +++ b/drivers/net/virtio/virtio_ethdev.c
> >> @@ -1848,7 +1848,14 @@ eth_virtio_dev_init(struct rte_eth_dev
> *eth_dev)
> >>   	struct virtio_hw *hw = eth_dev->data->dev_private;
> >>   	int ret;
> >>
> >> -	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct
> virtio_net_hdr_mrg_rxbuf));
> >> +	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) >
> RTE_PKTMBUF_HEADROOM) {
> >> +		PMD_INIT_LOG(ERR,
> >> +			"Not sufficient headroom required = %d, avail = %d",
> >> +			(int)sizeof(struct virtio_net_hdr_mrg_rxbuf),
> >> +			RTE_PKTMBUF_HEADROOM);
> >> +
> >> +		return -1;
> >> +	}
> >>
> >>   	eth_dev->dev_ops = &virtio_eth_dev_ops;
> >>
> >> --
> >> 2.17.1
> >>
> >