[v2,3/3] net/af_packet: get 'framesz' from the iface's MTU
Checks
Commit Message
Use the underlying MTU to calculate the framsize to be used for the mmap
RINGs. This is to make it more flexible on deployments with different
MTU requirements, instead of using a pre-defined value of 2048B.
If a 'framsz' option is provided, that value is used instead and the MTU
of the underlying interface is ignored.
Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
v2: Fix checkpatches.sh and check-git-log.sh warnings.
---
drivers/net/af_packet/rte_eth_af_packet.c | 33 ++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
Comments
On 11/20/2018 10:26 AM, Tiago Lam wrote:
> Use the underlying MTU to calculate the framsize to be used for the mmap
> RINGs. This is to make it more flexible on deployments with different
> MTU requirements, instead of using a pre-defined value of 2048B.
This behavior change should be documented in af_packet documentation which is
missing unfortunately.
Would you able to introduce the initial/basic af_packet doc to at least to
document device argument? If not please let me know, I can work on it.
>
> If a 'framsz' option is provided, that value is used instead and the MTU
> of the underlying interface is ignored.
>
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>
> v2: Fix checkpatches.sh and check-git-log.sh warnings.
<...>
> @@ -877,17 +877,40 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
> }
>
> ifnamelen = strlen(ifname);
> - if (ifnamelen >= sizeof(ifr.ifr_name)) {
> + if (ifnamelen < sizeof(ifr.ifr_name)) {
> + memcpy(ifr.ifr_name, ifname, ifnamelen);
> + ifr.ifr_name[ifnamelen] = '\0';
This can be replaces with strlcpy().
> + } else {
> RTE_LOG(ERR, PMD,
> "%s: I/F name too long (%s)\n",
> name, ifname);
> return -1;
> }
>
> + /*
> + * Base framesize on the MTU of the underlying interface, if no
> + * 'framesz' option is given
> + */
> + if (!framesize) {
> + if (ioctl(*sockfd, SIOCGIFMTU, &ifr) == -1) {
> + RTE_LOG(ERR, PMD,
> + "%s: ioctl failed (SIOCGIFMTU)",
> + name);
> + framesize = DFLT_FRAME_SIZE;
It may be good to add a log to say default frame size will be used, and perhaps
print out the default value.
On 11/27/2018 5:43 PM, Ferruh Yigit wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> Use the underlying MTU to calculate the framsize to be used for the mmap
>> RINGs. This is to make it more flexible on deployments with different
>> MTU requirements, instead of using a pre-defined value of 2048B.
>
> This behavior change should be documented in af_packet documentation which is
> missing unfortunately.
> Would you able to introduce the initial/basic af_packet doc to at least to
> document device argument? If not please let me know, I can work on it.
Hi John W.,
Is there any chance you can work on an af_packet driver documentation on DPDK as
a maintainer of the PMD?
Thanks,
ferruh
>
>>
>> If a 'framsz' option is provided, that value is used instead and the MTU
>> of the underlying interface is ignored.
>>
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
<...>
On 27/11/2018 17:43, Ferruh Yigit wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> Use the underlying MTU to calculate the framsize to be used for the mmap
>> RINGs. This is to make it more flexible on deployments with different
>> MTU requirements, instead of using a pre-defined value of 2048B.
>
> This behavior change should be documented in af_packet documentation which is
> missing unfortunately.
> Would you able to introduce the initial/basic af_packet doc to at least to
> document device argument? If not please let me know, I can work on it.
>
Thanks for the review, Ferruh.
Yeah, I don't mind cooking something up and submitting here for review;
I'll wait a couple of days for a reply from John W. before proceeding,
though.
But given there's no documentation for af_packet yet, do you prefer to
wait for that to be available, and apply it all together? Or could that
be applied later as part of another patch?
Tiago.
On 11/28/2018 1:12 PM, Lam, Tiago wrote:
> On 27/11/2018 17:43, Ferruh Yigit wrote:
>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>> RINGs. This is to make it more flexible on deployments with different
>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>
>> This behavior change should be documented in af_packet documentation which is
>> missing unfortunately.
>> Would you able to introduce the initial/basic af_packet doc to at least to
>> document device argument? If not please let me know, I can work on it.
>>
>
> Thanks for the review, Ferruh.
>
> Yeah, I don't mind cooking something up and submitting here for review;
> I'll wait a couple of days for a reply from John W. before proceeding,
> though.
Thanks, appreciated. Agreed to wait a little.
>
> But given there's no documentation for af_packet yet, do you prefer to
> wait for that to be available, and apply it all together? Or could that
> be applied later as part of another patch?
Both are OK, depends on your availability.
I think it is better, to show the history, first patch as the documentation
patch for existing behavior and your patch updating framsz usage (3/3) to update
that document as well.
Thanks,
ferruh
Hi Ferruh,
On 28/11/2018 13:33, Ferruh Yigit wrote:
> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>> RINGs. This is to make it more flexible on deployments with different
>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>
>>> This behavior change should be documented in af_packet documentation which is
>>> missing unfortunately.
>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>> document device argument? If not please let me know, I can work on it.
>>>
>>
>> Thanks for the review, Ferruh.
>>
>> Yeah, I don't mind cooking something up and submitting here for review;
>> I'll wait a couple of days for a reply from John W. before proceeding,
>> though.
>
> Thanks, appreciated. Agreed to wait a little.
>
>>
>> But given there's no documentation for af_packet yet, do you prefer to
>> wait for that to be available, and apply it all together? Or could that
>> be applied later as part of another patch?
>
> Both are OK, depends on your availability.
>
> I think it is better, to show the history, first patch as the documentation
> patch for existing behavior and your patch updating framsz usage (3/3) to update
> that document as well.
As agreed, I just sent a patch with an initial take on adding some docs
for af_packet. Once that's in I'll submit another revision of this
patchset, including an update to the documentation.
Just an aside, patch 1/3 of this series is a bugfix, it could go in
irrespective of the documentation, it seems.
Tiago.
On 12/17/2018 9:21 AM, Lam, Tiago wrote:
> Hi Ferruh,
>
> On 28/11/2018 13:33, Ferruh Yigit wrote:
>> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>>> RINGs. This is to make it more flexible on deployments with different
>>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>>
>>>> This behavior change should be documented in af_packet documentation which is
>>>> missing unfortunately.
>>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>>> document device argument? If not please let me know, I can work on it.
>>>>
>>>
>>> Thanks for the review, Ferruh.
>>>
>>> Yeah, I don't mind cooking something up and submitting here for review;
>>> I'll wait a couple of days for a reply from John W. before proceeding,
>>> though.
>>
>> Thanks, appreciated. Agreed to wait a little.
>>
>>>
>>> But given there's no documentation for af_packet yet, do you prefer to
>>> wait for that to be available, and apply it all together? Or could that
>>> be applied later as part of another patch?
>>
>> Both are OK, depends on your availability.
>>
>> I think it is better, to show the history, first patch as the documentation
>> patch for existing behavior and your patch updating framsz usage (3/3) to update
>> that document as well.
>
> As agreed, I just sent a patch with an initial take on adding some docs
> for af_packet. Once that's in I'll submit another revision of this
> patchset, including an update to the documentation.
>
> Just an aside, patch 1/3 of this series is a bugfix, it could go in
> irrespective of the documentation, it seems.
Agreed. Doc patch is merged, I will get first patch and will wait for a new
version for next.
On 11/28/2018 1:12 PM, Lam, Tiago wrote:
> On 27/11/2018 17:43, Ferruh Yigit wrote:
>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>> RINGs. This is to make it more flexible on deployments with different
>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>
>> This behavior change should be documented in af_packet documentation which is
>> missing unfortunately.
>> Would you able to introduce the initial/basic af_packet doc to at least to
>> document device argument? If not please let me know, I can work on it.
>>
>
> Thanks for the review, Ferruh.
>
> Yeah, I don't mind cooking something up and submitting here for review;
> I'll wait a couple of days for a reply from John W. before proceeding,
> though.
>
> But given there's no documentation for af_packet yet, do you prefer to
> wait for that to be available, and apply it all together? Or could that
> be applied later as part of another patch?
Unfortunately Tiago was not able to work on this task anymore.
And since Tiago's af_packet doc update already merged, I was planning to
complete the task and applied the comments into his patchset but had a few
questions, sharing here in case there are more interested people on task, cc'ed
Ian & Kevin.
Code is not functioning correct when there are gaps in the block, I mean when
block size is not multiple of frame size. There can be some assumption in the
code that memory is continuous.
Also I am not sure the benefit of the using MTU for this case. There are a few
restrictions, block size should be multiple of page size, it is 4K by default.
When using MTU, 1500, for frame size, instead of 2048 Bytes hardcoded value,
still only 2 frames fit into blocksize and there is no benefit, (and it is not
functioning with current code as I mentioned above.)
So this can be required for the cases MTU is bigger than 2048, not sure if this
is the concern of the patch.
Also it can provide some memory optimization when MTU is 1024 bytes or less, so
this provides memory optimization more than flexibility on deployment.
Hi Ian, Kevin,
Are you aware of any use case of this patch in OvS?
Thanks,
ferruh
On 2/18/2019 6:01 PM, Yigit, Ferruh wrote:
> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>> RINGs. This is to make it more flexible on deployments with different
>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>
>>> This behavior change should be documented in af_packet documentation which is
>>> missing unfortunately.
>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>> document device argument? If not please let me know, I can work on it.
>>>
>>
>> Thanks for the review, Ferruh.
>>
>> Yeah, I don't mind cooking something up and submitting here for review;
>> I'll wait a couple of days for a reply from John W. before proceeding,
>> though.
>>
>> But given there's no documentation for af_packet yet, do you prefer to
>> wait for that to be available, and apply it all together? Or could that
>> be applied later as part of another patch?
>
> Unfortunately Tiago was not able to work on this task anymore.
> And since Tiago's af_packet doc update already merged, I was planning to
> complete the task and applied the comments into his patchset but had a few
> questions, sharing here in case there are more interested people on task, cc'ed
> Ian & Kevin.
>
> Code is not functioning correct when there are gaps in the block, I mean when
> block size is not multiple of frame size. There can be some assumption in the
> code that memory is continuous.
>
> Also I am not sure the benefit of the using MTU for this case. There are a few
> restrictions, block size should be multiple of page size, it is 4K by default.
> When using MTU, 1500, for frame size, instead of 2048 Bytes hardcoded value,
> still only 2 frames fit into blocksize and there is no benefit, (and it is not
> functioning with current code as I mentioned above.)
> So this can be required for the cases MTU is bigger than 2048, not sure if this
> is the concern of the patch.
> Also it can provide some memory optimization when MTU is 1024 bytes or less, so
> this provides memory optimization more than flexibility on deployment.
>
> Hi Ian, Kevin,
>
> Are you aware of any use case of this patch in OvS?
It seems there is no follow up to this patchset, I am updating them as not
applicable.
For references, patches mentioned are:
https://patches.dpdk.org/user/todo/dpdk/?series=2498
@@ -788,7 +788,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
unsigned k_idx;
unsigned int blockcount;
unsigned int blocksize = DFLT_BLOCK_SIZE;
- unsigned int framesize = DFLT_FRAME_SIZE;
+ unsigned int framesize = 0;
unsigned int framecount = DFLT_FRAME_COUNT;
unsigned int qpairs = 1;
unsigned int qdisc_bypass = 1;
@@ -877,17 +877,40 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
}
ifnamelen = strlen(ifname);
- if (ifnamelen >= sizeof(ifr.ifr_name)) {
+ if (ifnamelen < sizeof(ifr.ifr_name)) {
+ memcpy(ifr.ifr_name, ifname, ifnamelen);
+ ifr.ifr_name[ifnamelen] = '\0';
+ } else {
RTE_LOG(ERR, PMD,
"%s: I/F name too long (%s)\n",
name, ifname);
return -1;
}
+ /*
+ * Base framesize on the MTU of the underlying interface, if no
+ * 'framesz' option is given
+ */
+ if (!framesize) {
+ if (ioctl(*sockfd, SIOCGIFMTU, &ifr) == -1) {
+ RTE_LOG(ERR, PMD,
+ "%s: ioctl failed (SIOCGIFMTU)",
+ name);
+ framesize = DFLT_FRAME_SIZE;
+ } else {
+ framesize = ifr.ifr_mtu;
+ /*
+ * Align to TPACKET_ALIGNMENT and add TPACKET2_HDRLEN,
+ * to account for the extra needed space
+ */
+ framesize = TPACKET_ALIGN(framesize + TPACKET2_HDRLEN);
+ }
+ }
+
if (framesize > blocksize) {
- PMD_LOG(ERR,
- "%s: AF_PACKET MMAP frame size exceeds block size!",
- name);
+ RTE_LOG(ERR, PMD,
+ "%s: AF_PACKET MMAP frame size (%u) exceeds block size (%u)!",
+ name, framesize, blocksize);
return -1;
}