Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[Michael Brown via ipxe-devel]

On 14/03/2023 16:45, MohamedShah R wrote:
Thanks for the quick response. I am new to the PXE source code and tried 
out the below changes  for the experimental testing in the netdev_close 
api and it works fine.
Why can't we have a new api netdev_reopen() which does the same logic of 
netdev_close (except the code below) and calls the netdev_open existing 
api and calls the netdev_reopen api only from the "apply_netdev_settings".

Would it have any other side-effects? Please correct me if I am wrong.

*Commented out the below code in netdev_close api *
  num_configs = table_num_entries ( NET_DEVICE_CONFIGURATORS );

for ( i = 0 ; i < num_configs ; i++ )

intf_close ( &netdev->configs[i].job, -ECANCELED );


As per my previous email: even with that hack in place, there will still 
be unwanted side effects from calling netdev->op->close() and 
netdev->op->open(), such as restarting autonegotiation on the link (with 
some drivers).


Michael

___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel

Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[MohamedShah R via ipxe-devel]

Hi Michael,

Thanks for the quick response. I am new to the PXE source code and tried
out the below changes  for the experimental testing in the netdev_close api
and it works fine.
Why can't we have a new api netdev_reopen() which does the same logic of
netdev_close (except the code below) and calls the netdev_open existing api
and calls the netdev_reopen api only from the "apply_netdev_settings".
Would it have any other side-effects? Please correct me if I am wrong.

*Commented out the below code in netdev_close api *
 num_configs = table_num_entries ( NET_DEVICE_CONFIGURATORS );
 for ( i = 0 ; i < num_configs ; i++ )
  intf_close ( &netdev->configs[i].job, -ECANCELED );

Regards
Mohamed Shah R
On Tue, Mar 14, 2023 at 7:15 PM Michael Brown via ipxe-devel <
ipxe-devel@lists.ipxe.org> wrote:

> On 13/03/2023 21:31, Michael Brown via ipxe-devel wrote:
> > There is what looks like a bug when using DHCP to change MTU: the change
> > of MTU requires the interface to be closed and reopened, but closing the
> > interface will cause the initiating DHCP transaction itself to be
> aborted.
> >
> > This probably requires the code in netdevice.c to be refactored slightly
> > to allow for a netdev_reopen() call, which skips the intf_close() calls.
>
> This is going to be very messy to support cleanly.  The combination of
> performing a close and reopen is fairly disruptive.  There are several
> other side effects from netdev_close() and netdev_open() (e.g. moving
> the device to the front of the "opened devices" list), which should
> probably be bypassed on an MTU change.
>
> I have a draft change in the "mtureset" branch.  However, even with this
> added complexity, there is still the problem that on several drivers a
> close/reopen will have other disruptive side effects such as restarting
> autonegotiation.
>
> I don't see a resolution to this any time soon.  Arguably the simplest
> workaround would be to remove the
>
> .tag = DHCP_MTU,
>
> from the definition of mtu_setting.  This would prevent the MTU from
> being changed automatically by DHCP.  A script could still set the MTU
> explicitly using e.g.
>
>set netX/mtu ${netX.dhcp/26}
>
> Michael
>
> ___
> ipxe-devel mailing list
> ipxe-devel@lists.ipxe.org
> https://lists.ipxe.org/mailman/listinfo/ipxe-devel
>
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel

Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[Michael Brown via ipxe-devel]

On 13/03/2023 21:31, Michael Brown via ipxe-devel wrote:
There is what looks like a bug when using DHCP to change MTU: the change 
of MTU requires the interface to be closed and reopened, but closing the 
interface will cause the initiating DHCP transaction itself to be aborted.


This probably requires the code in netdevice.c to be refactored slightly 
to allow for a netdev_reopen() call, which skips the intf_close() calls.


This is going to be very messy to support cleanly.  The combination of 
performing a close and reopen is fairly disruptive.  There are several 
other side effects from netdev_close() and netdev_open() (e.g. moving 
the device to the front of the "opened devices" list), which should 
probably be bypassed on an MTU change.


I have a draft change in the "mtureset" branch.  However, even with this 
added complexity, there is still the problem that on several drivers a 
close/reopen will have other disruptive side effects such as restarting 
autonegotiation.


I don't see a resolution to this any time soon.  Arguably the simplest 
workaround would be to remove the


   .tag = DHCP_MTU,

from the definition of mtu_setting.  This would prevent the MTU from 
being changed automatically by DHCP.  A script could still set the MTU 
explicitly using e.g.


  set netX/mtu ${netX.dhcp/26}

Michael

___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel

Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[Michael Brown via ipxe-devel]

On 13/03/2023 17:48, MohamedShah R wrote:

Hi Michael,
  Thanks for your response. Below is the flow- I have seen in the 
execution- it hits the intf_close as part of netdev_close called from 
apply_netdev_settings,.

https://github.com/ipxe/ipxe/blob/c4c03e5be867a9b7be4dc48fe6576deca1dce8d8/src/net/netdevice.c#L868
 
.


OK, now your original message makes sense to me.

The call to intf_close() that you see is closing any net device 
configuration methods (such as DHCP) that may still be running when the 
interface is closed.


The intf_close() call is simply sending a "close" message to the remote 
side of the interface (e.g. the DHCP protocol code).  As per the comment 
in the code, it does not actually close the interface at that point.


There is what looks like a bug when using DHCP to change MTU: the change 
of MTU requires the interface to be closed and reopened, but closing the 
interface will cause the initiating DHCP transaction itself to be aborted.


This probably requires the code in netdevice.c to be refactored slightly 
to allow for a netdev_reopen() call, which skips the intf_close() calls.


Michael

___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel

Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[MohamedShah R via ipxe-devel]

Hi Michael,
 Thanks for your response. Below is the flow- I have seen in the execution-
it hits the intf_close as part of netdev_close called from
apply_netdev_settings,.
https://github.com/ipxe/ipxe/blob/c4c03e5be867a9b7be4dc48fe6576deca1dce8d8/src/net/netdevice.c#L868
.

Please correct me if I am wrong.
Regards
Mohamed Shah R




On Mon, Mar 13, 2023 at 6:32 PM Michael Brown  wrote:

> On 13/03/2023 03:16, MohamedShah R via ipxe-devel wrote:
> >  1. If our device configures netdev->mtu as 1500  and
> > netdev->max_pkt_len is 9128(above 9k).
> >  2. If the DHCP server sends MTU size as 9K.   
> >
> > As per the code walk-through, I understand that this api
> > "staticintapply_netdev_settings” get invoke and call the below part of
> > code to handle the above scenario: In this case, as part of netdev_close
> > , it gets called the intf_close and it didn’t call the intf_init/openas
> > part of netdev_open api.
>
> I think you've got lost somewhere in the code, sorry.  There is no path
> from netdev_close() to intf_close().  The generic object interfaces
> handled by intf_init(), intf_close(), etc, exist at a higher layer in
> the stack.
>
> An MTU change for an open network interface via apply_netdev_setting()
> will result in the network devices .close() and .open() methods both
> being called.  Since receive buffers are allocated during .open(), this
> will result in all receive buffers having the correct size for the new MTU.
>
> Michael
>
>
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel

Re: [ipxe-devel] REG: Query on 9K MTU Testing flow

[Michael Brown via ipxe-devel]

On 13/03/2023 03:16, MohamedShah R via ipxe-devel wrote:

 1. If our device configures netdev->mtu as 1500  and
netdev->max_pkt_len is 9128(above 9k).
 2. If the DHCP server sends MTU size as 9K.   

As per the code walk-through, I understand that this api 
"staticintapply_netdev_settings” get invoke and call the below part of 
code to handle the above scenario: In this case, as part of netdev_close 
, it gets called the intf_close and it didn’t call the intf_init/openas 
part of netdev_open api.


I think you've got lost somewhere in the code, sorry.  There is no path 
from netdev_close() to intf_close().  The generic object interfaces 
handled by intf_init(), intf_close(), etc, exist at a higher layer in 
the stack.


An MTU change for an open network interface via apply_netdev_setting() 
will result in the network devices .close() and .open() methods both 
being called.  Since receive buffers are allocated during .open(), this 
will result in all receive buffers having the correct size for the new MTU.


Michael

___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo/ipxe-devel