| Message ID | 20201116135522.21791-4-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdev event handling + neighbour config | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote: > > Remove unnecessary function x25_kill_by_device. > -/* > - * Kill all bound sockets on a dropped device. > - */ > -static void x25_kill_by_device(struct net_device *dev) > -{ > - struct sock *s; > - > - write_lock_bh(&x25_list_lock); > - > - sk_for_each(s, &x25_list) > - if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev) > - x25_disconnect(s, ENETUNREACH, 0, 0); > - > - write_unlock_bh(&x25_list_lock); > -} > - > /* > * Handle device status changes. > */ > @@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block *this, unsigned long event, > case NETDEV_DOWN: > pr_debug("X.25: got event NETDEV_DOWN for device: %s\n", > dev->name); > - x25_kill_by_device(dev); > + nb = x25_get_neigh(dev); > + if (nb) { > + x25_kill_by_neigh(nb); > + x25_neigh_put(nb); > + } > x25_route_device_down(dev); > x25_link_device_down(dev); > break; This patch might not be entirely necessary. x25_kill_by_neigh and x25_kill_by_device are just two helper functions. One function takes nb as the argument and the other one takes dev as the argument. But they do almost the same things. It doesn't harm to keep both. In C++ we often have different functions with the same name doing almost the same things. The original code also seems to be a little more efficient than the new code.
On 2020-11-17 20:50, Xie He wrote: > On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> Remove unnecessary function x25_kill_by_device. > >> -/* >> - * Kill all bound sockets on a dropped device. >> - */ >> -static void x25_kill_by_device(struct net_device *dev) >> -{ >> - struct sock *s; >> - >> - write_lock_bh(&x25_list_lock); >> - >> - sk_for_each(s, &x25_list) >> - if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev >> == dev) >> - x25_disconnect(s, ENETUNREACH, 0, 0); >> - >> - write_unlock_bh(&x25_list_lock); >> -} >> - >> /* >> * Handle device status changes. >> */ >> @@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block >> *this, unsigned long event, >> case NETDEV_DOWN: >> pr_debug("X.25: got event NETDEV_DOWN for >> device: %s\n", >> dev->name); >> - x25_kill_by_device(dev); >> + nb = x25_get_neigh(dev); >> + if (nb) { >> + x25_kill_by_neigh(nb); >> + x25_neigh_put(nb); >> + } >> x25_route_device_down(dev); >> x25_link_device_down(dev); >> break; > > This patch might not be entirely necessary. x25_kill_by_neigh and > x25_kill_by_device are just two helper functions. One function takes > nb as the argument and the other one takes dev as the argument. But > they do almost the same things. It doesn't harm to keep both. In C++ > we often have different functions with the same name doing almost the > same things. > Well I don't like to have 2 functions doing the same thing. But after another look at this code, I've found that I also need to remove the call to x25_clear_forward_by_dev() in the function x25_route_device_down(). Otherwise, it will be called twice. > The original code also seems to be a little more efficient than the new > code. The only difference would be the x25_get_neigh() and x25_neigh_put() calls. That shouldn't cost to much.
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index 4c2a395fdbdb..25726120fcc7 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -212,22 +212,6 @@ static void x25_remove_socket(struct sock *sk) write_unlock_bh(&x25_list_lock); } -/* - * Kill all bound sockets on a dropped device. - */ -static void x25_kill_by_device(struct net_device *dev) -{ - struct sock *s; - - write_lock_bh(&x25_list_lock); - - sk_for_each(s, &x25_list) - if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev) - x25_disconnect(s, ENETUNREACH, 0, 0); - - write_unlock_bh(&x25_list_lock); -} - /* * Handle device status changes. */ @@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block *this, unsigned long event, case NETDEV_DOWN: pr_debug("X.25: got event NETDEV_DOWN for device: %s\n", dev->name); - x25_kill_by_device(dev); + nb = x25_get_neigh(dev); + if (nb) { + x25_kill_by_neigh(nb); + x25_neigh_put(nb); + } x25_route_device_down(dev); x25_link_device_down(dev); break;
Remove unnecessary function x25_kill_by_device. Signed-off-by: Martin Schiller <ms@dev.tdt.de> --- Change from v1: fix 'subject_prefix' warning --- net/x25/af_x25.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-)